Project

General

Profile

Feature #11976

Clarification on operator precedence/sequencing in fhicl grammar

Added by Ben Morgan over 3 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Target version:
Start date:
03/16/2016
Due date:
% Done:

100%

Estimated time:
2.00 h
Spent time:
Duration:

Description

I'm building fhicl-cpp with Clang and am getting warnings (promoted to errors by default with CAUTIOUS warning set from cetbuildtools) about operator precedence and sequencing from a few lines in parse.cc. Note that in the following the line numbers are four higher than than in source:fhiclcpp/parse.cc as I've added one additional pragma protection for Clang at the top of the file (This has zero effect on the warnings).

../fhiclcpp/parse.cc
../fhiclcpp/parse.cc:662:44: error: overloaded operator >> has higher precedence than comparison operator [-Werror,-Woverloaded-shift-op-parentheses]
         | (iter_pos >> lit("@sequence::") > noskip_qualname
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
../fhiclcpp/parse.cc:662:22: note: place parentheses around the '>>' expression to silence this warning
         | (iter_pos >> lit("@sequence::") > noskip_qualname
                     ^
            (                             )
../fhiclcpp/parse.cc:662:44: note: place parentheses around comparison expression to evaluate it first
         | (iter_pos >> lit("@sequence::") > noskip_qualname
                                           ^
                        (                                   )
../fhiclcpp/parse.cc:671:55: error: overloaded operator >> has higher precedence than comparison operator [-Werror,-Woverloaded-shift-op-parentheses]
                    | (iter_pos >> lit("@sequence::") > noskip_qualname
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
../fhiclcpp/parse.cc:671:33: note: place parentheses around the '>>' expression to silence this warning
                    | (iter_pos >> lit("@sequence::") > noskip_qualname
                                ^
                       (                             )
../fhiclcpp/parse.cc:671:55: note: place parentheses around comparison expression to evaluate it first
                    | (iter_pos >> lit("@sequence::") > noskip_qualname
                                                      ^
                                   (                                   )
../fhiclcpp/parse.cc:692:40: error: overloaded operator >> has higher precedence than comparison operator [-Werror,-Woverloaded-shift-op-parentheses]
        | (iter_pos >> lit("@table::") > noskip_qualname
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
../fhiclcpp/parse.cc:692:21: note: place parentheses around the '>>' expression to silence this warning
        | (iter_pos >> lit("@table::") > noskip_qualname
                    ^
           (                          )
../fhiclcpp/parse.cc:692:40: note: place parentheses around comparison expression to evaluate it first
        | (iter_pos >> lit("@table::") > noskip_qualname
                                       ^
                       (                                )
../fhiclcpp/parse.cc:703:38: error: multiple unsequenced modifications to '_val' [-Werror,-Wunsequenced]
    ((iter_pos >> vp.nil    ) [ _val = phx::bind(&xvalue_dp<iter_t>, ref(in_prolog), NIL     , qi::_2, qi::_1, ref(s)) ] |
                                     ^
../fhiclcpp/parse.cc:726:41: error: overloaded operator >> has higher precedence than comparison operator [-Werror,-Woverloaded-shift-op-parentheses]
         | (iter_pos >> lit("@table::") > noskip_qualname
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
../fhiclcpp/parse.cc:726:22: note: place parentheses around the '>>' expression to silence this warning
         | (iter_pos >> lit("@table::") > noskip_qualname
                     ^
            (                          )
../fhiclcpp/parse.cc:726:41: note: place parentheses around comparison expression to evaluate it first
         | (iter_pos >> lit("@table::") > noskip_qualname
                                        ^
                        (                                )
../fhiclcpp/parse.cc:737:50: error: overloaded operator >> has higher precedence than comparison operator [-Werror,-Woverloaded-shift-op-parentheses]
                  | (iter_pos >> lit("@table::") > noskip_qualname
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
../fhiclcpp/parse.cc:737:31: note: place parentheses around the '>>' expression to silence this warning
                  | (iter_pos >> lit("@table::") > noskip_qualname
                              ^
                     (                          )
../fhiclcpp/parse.cc:737:50: note: place parentheses around comparison expression to evaluate it first
                  | (iter_pos >> lit("@table::") > noskip_qualname
                                                 ^
                                 (                                )

I just wanted to cross check that the the correct fix would be to wrap the Expectation Operator in parentheses, i.e

iter_pos >> ( lit("@sequence::") > noskip_qualname )

That feels right given Spirit's parser operator behaviour and what I know of fhicl grammar...

There's also the "unsequenced modifications" warning in the above:

../fhiclcpp/parse.cc:703:38: error: multiple unsequenced modifications to '_val' [-Werror,-Wunsequenced]
    ((iter_pos >> vp.nil    ) [ _val = phx::bind(&xvalue_dp<iter_t>, ref(in_prolog), NIL     , qi::_2, qi::_1, ref(s)) ] |

I'm not sure if this is spurious or not - there's a similar construct at source:fhiclcpp/parse.cc#L608 which doesn't appear to cause an error.

Associated revisions

Revision cdf8859b (diff)
Added by Christopher Green over 3 years ago

Per issue #11976, mitigate Clang pragma and operator sequencing problems.

Revision db98ca13 (diff)
Added by Christopher Green over 3 years ago

Silence unsequenced modification warnings in clang per issue #11976.

History

#1 Updated by Ben Morgan over 3 years ago

Attached are two patches to provide a preliminary fix for this.

The first adds an additional pragma protection for Clang (for some reason, it takes -pedantic on the command line but only recognises -Wpedantic in diagnostics), plus giving the expectation operator precedence. All tests pass, barring those reported in #11860, so I believe the fix to not affect the grammar.

The second patch is to the same file, and is more speculative. This wraps the statement causing the "unsequenced modifications" error in a clang specific pragma, hence the "speculative" as this is simply to get Clang to compile it rather than any diagnosis of whether the warning is valid or not.

#2 Updated by Kyle Knoepfel over 3 years ago

  • Tracker changed from Support to Feature
  • Status changed from New to Accepted
  • Estimated time set to 2.00 h

Thanks for the patch. We will verify this works with GCC on the supported platforms and apply as appropriate.

#3 Updated by Kyle Knoepfel over 3 years ago

  • Assignee set to Christopher Green

#4 Updated by Christopher Green over 3 years ago

  • Status changed from Accepted to Resolved
  • Target version set to 2.01.00
  • % Done changed from 0 to 100

Analogous changes to mitigate the pragma and sequence problems were applied with cdf8859. Based on my understanding of the Boost-spirit semantics of > and >>, and our desire to have the parser fail early rather than late, I parenthesized the >> grouping instead of the > in your patch. Tests pass, so it appears to have no counter-intuitive effect under GCC. Please check that this change still cures the expected Clang warnings for you.

Patch 0004-Temp-fix-for-unsequenced-modification-Clang-errors.patch was applied as-is without problems with db98ca1. It appears this problem has been reported before on mailing lists back to 2013, with no response. I suspect it's a consequence of Boost Spirit's torturing of the operator semantics that is causing the complaint.

#5 Updated by Kyle Knoepfel over 3 years ago

  • Target version changed from 2.01.00 to 2.00.01

#6 Updated by Kyle Knoepfel over 3 years ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF