Feature #11976
Clarification on operator precedence/sequencing in fhicl grammar
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
Silence unsequenced modification warnings in clang per issue #11976.
History
#1 Updated by Ben Morgan almost 5 years ago
- File 0003-Fix-pragma-and-sequence-warnings-on-Clang.patch 0003-Fix-pragma-and-sequence-warnings-on-Clang.patch added
- File 0004-Temp-fix-for-unsequenced-modification-Clang-errors.patch 0004-Temp-fix-for-unsequenced-modification-Clang-errors.patch added
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 almost 5 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 almost 5 years ago
- Assignee set to Christopher Green
#4 Updated by Christopher Green almost 5 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 4 years ago
- Target version changed from 2.01.00 to 2.00.01
#6 Updated by Kyle Knoepfel over 4 years ago
- Status changed from Resolved to Closed
Per issue #11976, mitigate Clang pragma and operator sequencing problems.