[cfe-commits] r165283 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/parentheses.cpp

[Matt Beaumont-Gay]

On Fri, Oct 19, 2012 at 11:30 AM, David Blaikie <dblaikie at gmail.com> wrote:
> On Thu, Oct 18, 2012 at 6:16 PM, Matt Beaumont-Gay <matthewbg at google.com> wrote:
>> A little belated bikeshed-painting...
>>
>> On Thu, Oct 4, 2012 at 5:41 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=165283&r1=165282&r2=165283&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Oct  4 19:41:03 2012
>>> @@ -3863,6 +3863,11 @@
>>>  def note_logical_and_in_logical_or_silence : Note<
>>>    "place parentheses around the '&&' expression to silence this warning">;
>>>
>>> +def warn_addition_in_bitshift : Warning<
>>> +  "'%0' within '%1'">, InGroup<ShiftOpParentheses>;
>>
>> I have a hard time understanding what problem this warning is trying
>> to explain. Maybe we could phrase it like our other shift operator
>> precedence warning:
>> operator '?:' has lower precedence than '<<'; '<<' will be evaluated
>> first [-Werror,-Wparentheses]
>
> Fair point, improved as suggested in r166296. (pity we can't reuse the
> text of a warning because it's associated with a particular flag)

Thanks!

>
> I got the wording from the '||'+'&&' and '|'+'&' warning above this
> one. Should we change these too?

SGTM, but I don't have particularly strong feelings about it.

>
>>> +def note_addition_in_bitshift_silence : Note<
>>> +  "place parentheses around the '%0' expression to silence this warning">;
>>> +
>>>  def warn_self_assignment : Warning<
>>>    "explicitly assigning a variable of type %0 to itself">,
>>>    InGroup<SelfAssignment>, DefaultIgnore;
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=165283&r1=165282&r2=165283&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Oct  4 19:41:03 2012
>>> @@ -8570,6 +8570,20 @@
>>>    }
>>>  }
>>>
>>> +static void DiagnoseAdditionInShift(Sema &S, SourceLocation OpLoc,
>>> +                                    Expr *SubExpr, StringRef shift) {
>>> +  if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(SubExpr)) {
>>> +    if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) {
>>
>> Also, out of curiosity, why not warn on multiply/divide/mod?
>
> It's just what GCC supports. We could evaluate whether
> multiply/divide/mod are sufficiently error prone as to be worth
> diagnosing.

Given past observation of people not understanding the precedence of
<<, I'd say it's worth checking out.