[PATCH] D138914: Make evaluation of nested requirement consistent with requires expr.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2339
+Req->getConstraintExpr()->getSourceRange(), Satisfaction))
+  TransConstraint = Result[0];
+assert(!Trap.hasErrorOccurred() && "Substitution failures must be handled "

I have found a crash here when it access vector `Result` without checking the 
size first, leading to out of bounds memory access.  CReduce gave the following 
testcase:

```
template  struct b;
template  using d = b;
template  using f = d<__is_same(a, e)>;
template 
concept g = f::h;
template 
concept i = g;
template  class j {
  template 
  requires requires { requires i; }
  j();
};
template <> j();
```

`clang reduce.ii --std=c++20`

```
assertion failed at llvm/include/llvm/ADT/SmallVector.h:294 in reference 
llvm::SmallVectorTemplateCommon::operator[](size_type) [T = 
clang::Expr *]: idx < size()
...
...
(anonymous 
namespace)::TemplateInstantiator::TransformNestedRequirement(clang::concepts::NestedRequirement*)
 clang/lib/Sema/SemaTemplateInstantiate.cpp:2339:25
...
...
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138914/new/

https://reviews.llvm.org/D138914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136564: [clang] Instantiate NTTPs and template default arguments with sugar

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

I noticed some build failures after your commit.  I'm trying to get a 
standalone reproducer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136564/new/

https://reviews.llvm.org/D136564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/include/clang/AST/Type.h:3947
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }

rmaz wrote:
> aaron.ballman wrote:
> > It seems a bit odd to me that we only want to initialize one member of the 
> > bits and none of the rest.
> The reason I only set the `FastTypeQuals` is because the rest of the 
> `FunctionTypeBits` are not accessed in the base class or this class, except 
> for `ExtInfo` which is initialized in the base class.
This feels error-prone since any new classes derived from `FunctionType` will 
need to also have this.

I think the safer change is to modify `FunctionType::getFastTypeQuals()`.  If 
`this` is a `FunctionProtoType`, use `FastTypeQuals` to create a `Qualifiers`.  
If it is not, return a default created `Qualifiers`.  This way, we won't need 
to increase the access to data members.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D133586#3781468 , @rmaz wrote:

> In D133586#3781381 , @rtrieu wrote:
>
>> Do you have a test case for this?
>
> Was struggling to think of a good one. How about a test that repeatedly 
> generates a pcm for a func decl with no params and checks if the 
> DECL_FUNCTION record is the same?

Have you looked at clang/test/Modules/odr_hash.cpp?  It's where most of the ODR 
hash testing takes place by testing that Decls can be merged properly instead 
of checking the contents of pcm files..  Using `#if define`, it creates 
multiple modules from the same file.  I would suggest creating two functions in 
each of the modules, then in the main file, using the function to force it to 
be loaded from the modules and merged together.  The test should fail with the 
current Clang, but pass with your patch.  You may need to create your test file 
if you need different compiler options.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Do you have a test case for this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130510: Missing tautological compare warnings due to unary operators

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D130510#3728719 , @aaron.ballman 
wrote:

> In D130510#3727096 , @rtrieu wrote:
>
>> This patch has been moving back and forth between 
>> `IsIntegerLiteralConstantExpr` and `getIntegerLiteralSubexpressionValue`.  
>> The first function is preexisting and the second one is a new function.  The 
>> final patch seems to settle on using just 
>> `getIntegerLiteralSubexpressionValue`.  Can you explain why the existing 
>> function does not meet your needs?  It wasn't clear from the update messages 
>> why you went that way.
>
> The existing function returns whether the expression is an ICE (sort of), the 
> replacement function calculates the actual value and returns an optional 
> APInt so you can perform operations on it directly. That said, a future 
> refactoring could probably remove `IsIntegerLiteralConstantExpr()` and 
> replace it with `getIntegerLiteralSubexpressionValue()`, but the only caller 
> of `IsIntegerLiteralConstantExpr()` doesn't actually need the value at that 
> point.

In that case, I suggest adding a comment to pointing to the other function.  I 
expected that some day, someone will notice that the calculation for literals 
is slightly different between the two warnings and we should be helpful to 
them.  I have no other concerns.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130510/new/

https://reviews.llvm.org/D130510

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130510: Missing tautological compare warnings due to unary operators

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

This patch has been moving back and forth between 
`IsIntegerLiteralConstantExpr` and `getIntegerLiteralSubexpressionValue`.  The 
first function is preexisting and the second one is a new function.  The final 
patch seems to settle on using just `getIntegerLiteralSubexpressionValue`.  Can 
you explain why the existing function does not meet your needs?  It wasn't 
clear from the update messages why you went that way.

Besides that, there is added support for multiple unary operators, but only 
minus is tested.  Each one should have at least a positive and a negative test 
to show it is supported.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130510/new/

https://reviews.llvm.org/D130510

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131532: [Sema] Avoid isNullPointerConstant invocation

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

I think this is a reasonable step for improving compile times.  I put some 
suggestions for more descriptive names below (he said, after suggesting those 
names in the first place).

The description of this change should mention that expensive part is because 
`isNullPointerConstant` makes calls to a constant evaluator which we don't need.




Comment at: clang/lib/Sema/SemaChecking.cpp:13343
+  const Expr *NewE = E->IgnoreParenImpCasts();
+  bool GNUNull = isa(NewE);
+  bool NullPtr = NewE->getType()->isNullPtrType();

Let's call this `IsGNUNullExpr`



Comment at: clang/lib/Sema/SemaChecking.cpp:13344
+  bool GNUNull = isa(NewE);
+  bool NullPtr = NewE->getType()->isNullPtrType();
+  if (!GNUNull && !NullPtr) return;

And let's call this `HasNullPtrType`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131532/new/

https://reviews.llvm.org/D131532

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130510: Missing tautological compare warnings due to unary operators

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

  void foo(long x) {
if ((x & 1) == 1L) return;  // bad always false warning here
static_assert(sizeof(int) < sizeof(long), "long is bigger than int");
static_assert((long(15) & 1) == 1L, "proof that condition can be true");
  }

I found this false positive case when testing your new patch.  The condition is 
fine, but it gives an always false warning.  When fixed, this would be another 
good test case to include.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130510/new/

https://reviews.llvm.org/D130510

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130510: Missing tautological compare warnings due to unary operators

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Can you add my earlier test case or something like it to 
SemaCXX/warn-bitwise-compare.cpp ?

  template 
  void foo(int x) {
  bool b1 = (x & sizeof(T)) == 8;
  bool b2 = (x & I) == 8;
  bool b3 = (x & 4) == 8;  // only warn here
  }
  
  void run(int x) {
  foo<4, int>(8);
  }




Comment at: clang/lib/Analysis/CFG.cpp:58
 #include 
+#include 
 #include 

For debugging?

Have you tried `llvm::errs() << "message";` ?  A few streams are provided by 
LLVM support which most places have already, so no extra header is needed to 
make it work.



Comment at: clang/lib/Analysis/CFG.cpp:1023
+  // as -12.
+  llvm::APInt getIntegerLiteralSubexpressionValue(const Expr *E) {
+// UnaryOperator identification.

Is this any better than just having the callers use EvaluateAsInt themselves?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130510/new/

https://reviews.llvm.org/D130510

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130510: Missing tautological compare warnings due to unary operators

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D130510#3693494 , @aaron.ballman 
wrote:

> In D130510#3692654 , @rtrieu wrote:
>
>> Because of this, warnings should treat dependent expressions as non-constant 
>> even when they can be evaluated, so only `b3` should get a warning.  This is 
>> causing the warning to be emitted on code heavy in template 
>> meta-programming, such as array libraries.  Please revert or fix.
>
> Yeah, I agree. Unfortunately, the CFG makes this exceptionally difficult 
> because it walks over the instantiated code, so we've lost that the original 
> expression was dependent by the time we get to checking the binary 
> expressions. The original code worked by virtue of overfitting to *just* 
> integer literals.

Not being able to detect when expressions are dependent inside template 
instantiations has been a pain for warnings since forever.

>> I believe that evaluating the expression would make this warning too broad 
>> and would need more testing that what was included here.  Only handling 
>> UnaryOperator with IntegerLiteral sub-expression makes more sense for the 
>> warning, and adding in any new cases if we find them.
>
> I agree that the warning is too broad right now and that's unintentional. 
> However, manually handling every single case in the CFG as something special 
> is fragile and what got us this bug in the first place. We have a constant 
> expression evaluator (two, actually) and we shouldn't have to reimplement it 
> a third time in the CFG. However, asking a GSoC mentee to address that is 
> well beyond the scope of what they should be working on. So for now I'm going 
> to revert this change, reopen the issue, and we'll discuss the next steps 
> off-list with Usman.

I agree, the CFG should be as broadly applicable as possible, so using an 
evaluator there is fine.  Manually handling every single case may be needed to 
keep the warning under control.  It's possible to put that handling to the Sema 
side of things, right before the warning is emitted.  There's already a filter 
for macros, so maybe adding the filtering logic there would be a good fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130510/new/

https://reviews.llvm.org/D130510

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130510: Missing tautological compare warnings due to unary operators

[Richard Trieu via Phabricator via cfe-commits]

rtrieu reopened this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

This warning is now flagging some code which I believe is a false positive.  In 
particular, when template arguments are involved, their values can be 
calculated during template instantiation, but they should be treated as 
variables instead of constants.  For example:

  template 
  void foo(int x) {
  bool b1 = (x & sizeof(T)) == 8;
  bool b2 = (x & I) == 8;
  bool b3 = (x & 4) == 8;
  }
  
  void run(int x) {
  foo<4, int>(8);
  }

In this instantiation, x is and'ed with the value 4 in different ways.  
`sizeof(T)` is type-dependent, `I` is value-dependent, and `4` is an integer 
literal.  With your code, each of them would produce a warning.  However, the 
first two values of 4 will change in different template instantiations, so the 
warning is not useful when it is okay for some instantiations to have constant 
values.  Because of this, warnings should treat dependent expressions as 
non-constant even when they can be evaluated, so only `b3` should get a 
warning.  This is causing the warning to be emitted on code heavy in template 
meta-programming, such as array libraries.  Please revert or fix.

I believe that evaluating the expression would make this warning too broad and 
would need more testing that what was included here.  Only handling 
UnaryOperator with IntegerLiteral sub-expression makes more sense for the 
warning, and adding in any new cases if we find them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130510/new/

https://reviews.llvm.org/D130510

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/AST/QualTypeNames.cpp:455
+  if (const auto *UT = QT->getAs()) {
+return getFullyQualifiedType(UT->getUnderlyingType(), Ctx,
+ WithGlobalNsPrefix);

Moving this down here means when the ElaboratedType is stripped off, its 
Qualifers aren't preserved in the underlying type.  rGfb7fa27f92ca has a fix to 
reattach the discarded Qualifiers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112374/new/

https://reviews.llvm.org/D112374

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

This seems to be acting weird in template instantations.  Here's an example 
where the lambda only errors inside a template.

  template 
  int foo(int x = 0) {
  auto lambda = [x = x+1]() -> decltype(x) {
  return x;
  };
  return -1;
  }
  
  // no template
  int bar(int x = 0) {
  auto lambda = [x = x+1]() -> decltype(x) {
  return x;
  };
  return -1;
  }
  
  int a = foo();  // error
  int b = bar();  // no  error



  /tmp/lambda.cc:4:16: error: variable 'x' cannot be implicitly captured in a 
lambda with no capture-default specified
  return x;
 ^
  /tmp/lambda.cc:17:9: note: in instantiation of function template 
specialization 'foo' requested here
  int a = foo();  // error
  ^
  /tmp/lambda.cc:3:20: note: 'x' declared here
  auto lambda = [x = x+1]() -> decltype(x) {
 ^
  /tmp/lambda.cc:3:19: note: lambda expression begins here
  auto lambda = [x = x+1]() -> decltype(x) {
^
  /tmp/lambda.cc:3:27: note: capture 'x' by value
  auto lambda = [x = x+1]() -> decltype(x) {
^
, x
  /tmp/lambda.cc:3:27: note: capture 'x' by reference
  auto lambda = [x = x+1]() -> decltype(x) {
^
, 
  /tmp/lambda.cc:3:20: note: default capture by value
  auto lambda = [x = x+1]() -> decltype(x) {
 ^
 =, 
  /tmp/lambda.cc:3:20: note: default capture by reference
  auto lambda = [x = x+1]() -> decltype(x) {
 ^
 &, 
  1 error generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119136/new/

https://reviews.llvm.org/D119136

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119496: [Clang][OpaquePtr] Remove calls to deprecated Address constructor

[Richard Trieu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd5c314cdf43a: [Clang][OpaquePtr] Remove deprecated Address 
constructor calls (authored by rtrieu).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119496/new/

https://reviews.llvm.org/D119496

Files:
  clang/lib/CodeGen/CGExpr.cpp

Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -407,7 +407,7 @@
   GV->getValueType()->getPointerTo(
   CGF.getContext().getTargetAddressSpace(LangAS::Default)));
 // FIXME: Should we put the new global into a COMDAT?
-return Address(C, alignment);
+return Address(C, GV->getValueType(), alignment);
   }
 return CGF.CreateMemTemp(Ty, "ref.tmp", Alloca);
   }
@@ -441,10 +441,10 @@
   ownership != Qualifiers::OCL_ExplicitNone) {
 Address Object = createReferenceTemporary(*this, M, E);
 if (auto *Var = dyn_cast(Object.getPointer())) {
-  Object = Address(llvm::ConstantExpr::getBitCast(Var,
-   ConvertTypeForMem(E->getType())
- ->getPointerTo(Object.getAddressSpace())),
-   Object.getAlignment());
+  llvm::Type *Ty = ConvertTypeForMem(E->getType());
+  Object = Address(llvm::ConstantExpr::getBitCast(
+   Var, Ty->getPointerTo(Object.getAddressSpace())),
+   Ty, Object.getAlignment());
 
   // createReferenceTemporary will promote the temporary to a global with a
   // constant initializer if it can.  It can only do this to a value of
@@ -499,9 +499,11 @@
   Address Object = createReferenceTemporary(*this, M, E, );
   if (auto *Var = dyn_cast(
   Object.getPointer()->stripPointerCasts())) {
+llvm::Type *TemporaryType = ConvertTypeForMem(E->getType());
 Object = Address(llvm::ConstantExpr::getBitCast(
  cast(Object.getPointer()),
- ConvertTypeForMem(E->getType())->getPointerTo()),
+ TemporaryType->getPointerTo()),
+ TemporaryType,
  Object.getAlignment());
 // If the temporary is a global and has a constant initializer or is a
 // constant temporary that we promoted to a global, we may have already
@@ -1208,9 +1210,10 @@
 LValue CodeGenFunction::EmitUnsupportedLValue(const Expr *E,
   const char *Name) {
   ErrorUnsupported(E, Name);
-  llvm::Type *Ty = llvm::PointerType::getUnqual(ConvertType(E->getType()));
-  return MakeAddrLValue(Address(llvm::UndefValue::get(Ty), CharUnits::One()),
-E->getType());
+  llvm::Type *ElTy = ConvertType(E->getType());
+  llvm::Type *Ty = llvm::PointerType::getUnqual(ElTy);
+  return MakeAddrLValue(
+  Address(llvm::UndefValue::get(Ty), ElTy, CharUnits::One()), E->getType());
 }
 
 bool CodeGenFunction::IsWrappedCXXThis(const Expr *Obj) {
@@ -2741,8 +2744,10 @@
 LValue CapLVal =
 EmitCapturedFieldLValue(*this, CapturedStmtInfo->lookup(VD),
 CapturedStmtInfo->getContextValue());
+Address LValueAddress = CapLVal.getAddress(*this);
 CapLVal = MakeAddrLValue(
-Address(CapLVal.getPointer(*this), getContext().getDeclAlign(VD)),
+Address(LValueAddress.getPointer(), LValueAddress.getElementType(),
+getContext().getDeclAlign(VD)),
 CapLVal.getType(), LValueBaseInfo(AlignmentSource::Decl),
 CapLVal.getTBAAInfo());
 // Mark lvalue as nontemporal if the variable is marked as nontemporal
@@ -3431,7 +3436,8 @@
   CfiCheckFailDataTy,
   Builder.CreatePointerCast(Data, CfiCheckFailDataTy->getPointerTo(0)), 0,
   0);
-  Address CheckKindAddr(V, getIntAlign());
+
+  Address CheckKindAddr(V, Int8Ty, getIntAlign());
   llvm::Value *CheckKind = Builder.CreateLoad(CheckKindAddr);
 
   llvm::Value *AllVtables = llvm::MetadataAsValue::get(
@@ -3817,7 +3823,7 @@
 llvm::Value *EltPtr =
 emitArraySubscriptGEP(*this, Addr.getElementType(), Addr.getPointer(),
   ScaledIdx, false, SignedIndices, E->getExprLoc());
-Addr = Address(EltPtr, EltAlign);
+Addr = Address(EltPtr, Addr.getElementType(), EltAlign);
 
 // Cast back.
 Addr = Builder.CreateBitCast(Addr, OrigBaseTy);
@@ -3917,7 +3923,8 @@
 CGF.CGM.getNaturalTypeAlignment(ElTy, , );
 BaseInfo.mergeForCast(TypeBaseInfo);
 TBAAInfo = CGF.CGM.mergeTBAAInfoForCast(TBAAInfo, TypeTBAAInfo);
-return Address(CGF.Builder.CreateLoad(BaseLVal.getAddress(CGF)), Align);
+return Address(CGF.Builder.CreateLoad(BaseLVal.getAddress(CGF)),
+   CGF.ConvertTypeForMem(ElTy), 

[PATCH] D119496: [Clang][OpaquePtr] Remove calls to deprecated Address constructor

[Richard Trieu via Phabricator via cfe-commits]

rtrieu created this revision.
rtrieu added reviewers: cfe-commits, aeubanks.
rtrieu requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.
Herald added a project: clang.

Explicitly specify element type when constructing Address in CGExpr.cpp.  
There's one more in EmitLoadOfPointer that was a little trickier, so it was not 
changed.  That one seems to only affect OpenMP tests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119496

Files:
  clang/lib/CodeGen/CGExpr.cpp

Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -407,7 +407,7 @@
   GV->getValueType()->getPointerTo(
   CGF.getContext().getTargetAddressSpace(LangAS::Default)));
 // FIXME: Should we put the new global into a COMDAT?
-return Address(C, alignment);
+return Address(C, GV->getValueType(), alignment);
   }
 return CGF.CreateMemTemp(Ty, "ref.tmp", Alloca);
   }
@@ -441,10 +441,10 @@
   ownership != Qualifiers::OCL_ExplicitNone) {
 Address Object = createReferenceTemporary(*this, M, E);
 if (auto *Var = dyn_cast(Object.getPointer())) {
-  Object = Address(llvm::ConstantExpr::getBitCast(Var,
-   ConvertTypeForMem(E->getType())
- ->getPointerTo(Object.getAddressSpace())),
-   Object.getAlignment());
+  llvm::Type *Ty = ConvertTypeForMem(E->getType());
+  Object = Address(llvm::ConstantExpr::getBitCast(
+   Var, Ty->getPointerTo(Object.getAddressSpace())),
+   Ty, Object.getAlignment());
 
   // createReferenceTemporary will promote the temporary to a global with a
   // constant initializer if it can.  It can only do this to a value of
@@ -499,9 +499,11 @@
   Address Object = createReferenceTemporary(*this, M, E, );
   if (auto *Var = dyn_cast(
   Object.getPointer()->stripPointerCasts())) {
+llvm::Type *TemporaryType = ConvertTypeForMem(E->getType());
 Object = Address(llvm::ConstantExpr::getBitCast(
  cast(Object.getPointer()),
- ConvertTypeForMem(E->getType())->getPointerTo()),
+ TemporaryType->getPointerTo()),
+ TemporaryType,
  Object.getAlignment());
 // If the temporary is a global and has a constant initializer or is a
 // constant temporary that we promoted to a global, we may have already
@@ -1208,9 +1210,10 @@
 LValue CodeGenFunction::EmitUnsupportedLValue(const Expr *E,
   const char *Name) {
   ErrorUnsupported(E, Name);
-  llvm::Type *Ty = llvm::PointerType::getUnqual(ConvertType(E->getType()));
-  return MakeAddrLValue(Address(llvm::UndefValue::get(Ty), CharUnits::One()),
-E->getType());
+  llvm::Type *ElTy = ConvertType(E->getType());
+  llvm::Type *Ty = llvm::PointerType::getUnqual(ElTy);
+  return MakeAddrLValue(
+  Address(llvm::UndefValue::get(Ty), ElTy, CharUnits::One()), E->getType());
 }
 
 bool CodeGenFunction::IsWrappedCXXThis(const Expr *Obj) {
@@ -2741,8 +2744,10 @@
 LValue CapLVal =
 EmitCapturedFieldLValue(*this, CapturedStmtInfo->lookup(VD),
 CapturedStmtInfo->getContextValue());
+Address LValueAddress = CapLVal.getAddress(*this);
 CapLVal = MakeAddrLValue(
-Address(CapLVal.getPointer(*this), getContext().getDeclAlign(VD)),
+Address(LValueAddress.getPointer(), LValueAddress.getElementType(),
+getContext().getDeclAlign(VD)),
 CapLVal.getType(), LValueBaseInfo(AlignmentSource::Decl),
 CapLVal.getTBAAInfo());
 // Mark lvalue as nontemporal if the variable is marked as nontemporal
@@ -3431,7 +3436,8 @@
   CfiCheckFailDataTy,
   Builder.CreatePointerCast(Data, CfiCheckFailDataTy->getPointerTo(0)), 0,
   0);
-  Address CheckKindAddr(V, getIntAlign());
+
+  Address CheckKindAddr(V, Int8Ty, getIntAlign());
   llvm::Value *CheckKind = Builder.CreateLoad(CheckKindAddr);
 
   llvm::Value *AllVtables = llvm::MetadataAsValue::get(
@@ -3817,7 +3823,7 @@
 llvm::Value *EltPtr =
 emitArraySubscriptGEP(*this, Addr.getElementType(), Addr.getPointer(),
   ScaledIdx, false, SignedIndices, E->getExprLoc());
-Addr = Address(EltPtr, EltAlign);
+Addr = Address(EltPtr, Addr.getElementType(), EltAlign);
 
 // Cast back.
 Addr = Builder.CreateBitCast(Addr, OrigBaseTy);
@@ -3917,7 +3923,8 @@
 CGF.CGM.getNaturalTypeAlignment(ElTy, , );
 BaseInfo.mergeForCast(TypeBaseInfo);
 TBAAInfo = CGF.CGM.mergeTBAAInfoForCast(TBAAInfo, TypeTBAAInfo);
-return Address(CGF.Builder.CreateLoad(BaseLVal.getAddress(CGF)), Align);
+ 

[PATCH] D117376: Remove reference type when checking const structs

[Richard Trieu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbe2147db054e: Remove reference type when checking const 
structs (authored by rtrieu).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117376/new/

https://reviews.llvm.org/D117376

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/merge-all-constants-references.cpp


Index: clang/test/CodeGenCXX/merge-all-constants-references.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/merge-all-constants-references.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fmerge-all-constants 
%s -o /dev/null
+
+struct A {
+};
+
+struct B {
+  const struct A& a = {};
+};
+
+void Test(const struct B&);
+
+void Run() {
+  Test({});
+}
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -851,6 +851,7 @@
 }
 
 llvm::Constant *ConstStructBuilder::Finalize(QualType Type) {
+  Type = Type.getNonReferenceType();
   RecordDecl *RD = Type->castAs()->getDecl();
   llvm::Type *ValTy = CGM.getTypes().ConvertType(Type);
   return Builder.build(ValTy, RD->hasFlexibleArrayMember());


Index: clang/test/CodeGenCXX/merge-all-constants-references.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/merge-all-constants-references.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fmerge-all-constants %s -o /dev/null
+
+struct A {
+};
+
+struct B {
+  const struct A& a = {};
+};
+
+void Test(const struct B&);
+
+void Run() {
+  Test({});
+}
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -851,6 +851,7 @@
 }
 
 llvm::Constant *ConstStructBuilder::Finalize(QualType Type) {
+  Type = Type.getNonReferenceType();
   RecordDecl *RD = Type->castAs()->getDecl();
   llvm::Type *ValTy = CGM.getTypes().ConvertType(Type);
   return Builder.build(ValTy, RD->hasFlexibleArrayMember());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117376: Remove reference type when checking const structs

[Richard Trieu via Phabricator via cfe-commits]

rtrieu created this revision.
rtrieu added reviewers: rsmith, cfe-commits.
rtrieu requested review of this revision.
Herald added a project: clang.

ConstStructBuilder::Finalize in CGExprConstant.ccp assumes that the passed in 
QualType is a RecordType.  In some instances, the type is a reference to a 
RecordType and the reference needs to be removed first.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117376

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/merge-all-constants-references.cpp


Index: clang/test/CodeGenCXX/merge-all-constants-references.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/merge-all-constants-references.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fmerge-all-constants 
%s -o /dev/null
+
+struct A {
+};
+
+struct B {
+  const struct A& a = {};
+};
+
+void Test(const struct B&);
+
+void Run() {
+  Test({});
+}
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -851,6 +851,7 @@
 }
 
 llvm::Constant *ConstStructBuilder::Finalize(QualType Type) {
+  Type = Type.getNonReferenceType();
   RecordDecl *RD = Type->castAs()->getDecl();
   llvm::Type *ValTy = CGM.getTypes().ConvertType(Type);
   return Builder.build(ValTy, RD->hasFlexibleArrayMember());


Index: clang/test/CodeGenCXX/merge-all-constants-references.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/merge-all-constants-references.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fmerge-all-constants %s -o /dev/null
+
+struct A {
+};
+
+struct B {
+  const struct A& a = {};
+};
+
+void Test(const struct B&);
+
+void Run() {
+  Test({});
+}
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -851,6 +851,7 @@
 }
 
 llvm::Constant *ConstStructBuilder::Finalize(QualType Type) {
+  Type = Type.getNonReferenceType();
   RecordDecl *RD = Type->castAs()->getDecl();
   llvm::Type *ValTy = CGM.getTypes().ConvertType(Type);
   return Builder.build(ValTy, RD->hasFlexibleArrayMember());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114382: [clang] Fix wrong -Wunused-local-typedef warning within a template function

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/test/Modules/odr_hash.cpp:4288
 S s;
+// expected-error@first.h:* {{'ParameterTest::S::Foo' has different 
definitions in different modules; definition in module 'FirstModule' first 
difference is 1st parameter with name ''}}
+// expected-note@second.h:* {{but in 'SecondModule' found 1st parameter with 
name 'asdf'}}

krisb wrote:
> I'm not sure what was the original intent of this test (i.e. whether it 
> intentionally tests the fact that there is no error on an uninstantiated 
> static member function). As well as it doesn't clear to me what is the role 
> of the unused typedef here, but it starts triggering the error because of 
> redecls() call on isReferenced() added by this patch.
I've checked out the ODR Hashing versus the other cases.   The new error is in 
line with the other behavior.  Having this new error is okay.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114382/new/

https://reviews.llvm.org/D114382

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108794: Fully qualify template template parameters when printing

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

In D108794#2978134 , @dblaikie wrote:

> Ping
>
> In D108794#2976063 , @rtrieu wrote:
>
>> It looks like a strict improvement on printing and most callers using the 
>> default args won't need to be updated.
>>
>> There's one more function call that should be updated:
>> https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/DumpAST.cpp#L298
>>
>> Fixing that and the comment and this should be good to go in.
>
> Ah, thanks for catching that (just running `check-clang` doesn't catch this, 
> and it doesn't seem like there's a `check-clang-tools-extra` - any idea if 
> there's something narrower than `check-all` that would run clang-tools-extra 
> tests?). Updated that caller to preserve the existing unqualified behavior.

I don't know if there is a better way to check.  I found it by doing a quick 
search over the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108794/new/

https://reviews.llvm.org/D108794

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108794: Fully qualify template template parameters when printing

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

It looks like a strict improvement on printing and most callers using the 
default args won't need to be updated.

There's one more function call that should be updated:
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/DumpAST.cpp#L298

Fixing that and the comment and this should be good to go in.




Comment at: clang/include/clang/AST/TemplateName.h:318-320
   /// \param SuppressNNS if true, don't print the
   /// nested-name-specifier that precedes the template name (if it has
   /// one).

Update this comment to reflect the enum.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108794/new/

https://reviews.llvm.org/D108794

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101387: remove single quotes around sugguestion diagnostic

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D101387#2720500 , @MaskRay wrote:

> @rtrieu Do we have a way appending arbitrary messages to a diagnostic 
> template?

Not an arbitrary number.  The diagnostic format string is indexed, so it 
diagnostic string needs to know ahead of time how many arguments that will be 
passed to it.

The format types can be nested so you can do:

  "Some types: %select{|%1|%1 %2|%1 %2 %3|%1 %2 %3 and more}0"

Then you can pass {0,1,2,3} first, then follow with (0,1,2,3) strings.  And 
passing 4 first with 3 strings gets the "and more" attached to the end.  
Otherwise, the string concatenation will need to be done before passing to the 
diagnostic.

For this case, I think to capture the variations used, we could use:

  "invalid value '%1' in '%0', %select{|for %3,}2 $plural{0:value must be 
%5|[1,4]:valid argument for '%0' are:%select{|%5|%5 %6|%5 %6 %7|%5 %6 %7 %8}4}4"

%0 - flag name
%1 - invalid value
%2 - if true, add extra "for %3"
%3 - string for extra "for", needed but ignored when %2 is false
%4 - count for valid arguments.  If 0, arbitrary string after "value must be"
%5-%8 - valid arguments

This supports up to 4 types and an arbitrary string.  Unifying the flang and 
clang diagnostics could simplify the string a bit more.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101387/new/

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/Sema/SemaChecking.cpp:10166
+  /// The number of bits active in the int. Note that this includes exactly one
+  /// sign bit if !NoNegative.
   unsigned Width;

NonNegative


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85574: [Sema] Fix missing warning on initializer lists on field initializers with overloaded operators

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85574/new/

https://reviews.llvm.org/D85574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85574: [Sema] Fix missing warning on initializer lists on field initializers with overloaded operators

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3584-3585
 
   if (!isa(Base->IgnoreParenImpCasts()))
 return;
 

Does the warning work if it was changed to be "Visit(Base);" before the return 
here instead of the change above?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85574/new/

https://reviews.llvm.org/D85574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85287: Extend -Wtautological-bitwise-compare "bitwise or with non-zero value" warnings

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D85287#2199490 , @sberg wrote:

> In D85287#2199463 , @rtrieu wrote:
>
>> Also, have you tried running warning over a codebase?
>
> As I wrote: "At least building LibreOffice with this change caused no false 
> positives."  (Or maybe I misunderstand your question.)

My bad.  I read too fast and thought you said you ran the scan over LibreOffice 
instead of your warning.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85287/new/

https://reviews.llvm.org/D85287

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85287: Extend -Wtautological-bitwise-compare "bitwise or with non-zero value" warnings

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a subscriber: AndersRonnholm.
rtrieu added a comment.

I looked back on the commits and while I did commit, I did it on the behalf of 
Anders Rönnholm, who did not have commit access at the time.  I haven't seen 
activity from Anders in a while, but added to subscribers just in case.

Way back then, the warning only did operands of a DeclRefExpr and an 
IntegerLiteral.  Over time, that has been extended, case by case, to include 
whatever new cases people can think up.  I don't mind extending the warnings, 
but we need to be mindful of how the warnings appear.  If the sub-expression 
becomes too large, it will be difficult for the user to understand where the 
problem is and which constants the compiler is talking about.  We may already 
be at that point.  The example could have a more complex initializer for the 
constant variables, and the warning would be harder to follow.  Maybe we also 
look at the variable initializers and only allow for simple ones.  I need to 
give this some more thought.

> To avoid potential further false positives, restrict this change only to the
> "bitwise or with non-zero value" warnings while keeping all other
> -Wtautological-bitwise-compare warnings as-is, at least for now.

Are you planning to allow this change to other warnings that use the same 
helper functions?
Also, have you tried running warning over a codebase?




Comment at: clang/lib/Analysis/CFG.cpp:96-97
 
-/// Helper for tryNormalizeBinaryOperator. Attempts to extract an 
IntegerLiteral
-/// constant expression or EnumConstantDecl from the given Expr. If it fails,
+/// Helper for tryNormalizeBinaryOperator. Attempts to extract a suitable
+/// integer or enum constant from the given Expr. If it fails,
 /// returns nullptr.

The original comment specifies the allowed Expr's by the specific AST nodes 
they represent.  Please use that.  I think "IntegerLiteral constant expression, 
EnumConstantDecl, or constant value VarDecl" would work.



Comment at: clang/lib/Analysis/CFG.cpp:110
+  if (VD->isUsableInConstantExpressions(Ctx))
+return DR;
+  }

IntergerLiteral and EnumConstantDecl are known to have integer types.  However, 
it is possible for a VarDecl to have other types.  There should be a check for 
integer types here.



Comment at: clang/lib/Analysis/CFG.cpp:175
 
-  assert(isa(DC1) && isa(DC2));
-  return DC1 == DC2;
+  return false;
 }

Need a comment here about how tryTransformToIntOrEnumConstant also allows a 
DeclRefExpr to have a constant VarDecl, but this case is currently excluded for 
this warning.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85287/new/

https://reviews.llvm.org/D85287

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85256: Add -Wtautological-value-range-compare warning.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85256/new/

https://reviews.llvm.org/D85256

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79548: Fix false positive with warning -Wnon-c-typedef-for-linkage

[Richard Trieu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4ae537c2220f: Fix false positive with 
-Wnon-c-typedef-for-linkage (authored by rtrieu).

Changed prior to commit:
  https://reviews.llvm.org/D79548?vs=262536=262808#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79548/new/

https://reviews.llvm.org/D79548

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/anonymous-struct.cpp


Index: clang/test/SemaCXX/anonymous-struct.cpp
===
--- clang/test/SemaCXX/anonymous-struct.cpp
+++ clang/test/SemaCXX/anonymous-struct.cpp
@@ -133,3 +133,23 @@
 int arr[ ? 1 : 2];
   } C; // expected-note {{by this typedef}}
 }
+
+namespace ImplicitDecls {
+struct Destructor {
+  ~Destructor() {}
+};
+typedef struct {
+} Empty;
+
+typedef struct {
+  Destructor x;
+} A;
+
+typedef struct {
+  Empty E;
+} B;
+
+typedef struct {
+  const Empty E;
+} C;
+} // namespace ImplicitDecls
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -4425,8 +4425,11 @@
 isa(D))
   continue;
 auto *MemberRD = dyn_cast(D);
-if (!MemberRD)
+if (!MemberRD) {
+  if (D->isImplicit())
+continue;
   return {NonCLikeKind::OtherMember, D->getSourceRange()};
+}
 
 //  -- contain a lambda-expression,
 if (MemberRD->isLambda())


Index: clang/test/SemaCXX/anonymous-struct.cpp
===
--- clang/test/SemaCXX/anonymous-struct.cpp
+++ clang/test/SemaCXX/anonymous-struct.cpp
@@ -133,3 +133,23 @@
 int arr[ ? 1 : 2];
   } C; // expected-note {{by this typedef}}
 }
+
+namespace ImplicitDecls {
+struct Destructor {
+  ~Destructor() {}
+};
+typedef struct {
+} Empty;
+
+typedef struct {
+  Destructor x;
+} A;
+
+typedef struct {
+  Empty E;
+} B;
+
+typedef struct {
+  const Empty E;
+} C;
+} // namespace ImplicitDecls
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -4425,8 +4425,11 @@
 isa(D))
   continue;
 auto *MemberRD = dyn_cast(D);
-if (!MemberRD)
+if (!MemberRD) {
+  if (D->isImplicit())
+continue;
   return {NonCLikeKind::OtherMember, D->getSourceRange()};
+}
 
 //  -- contain a lambda-expression,
 if (MemberRD->isLambda())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79548: Fix false positive with warning -Wnon-c-typedef-for-linkage

[Richard Trieu via Phabricator via cfe-commits]

rtrieu created this revision.
rtrieu added a reviewer: rsmith.
Herald added a project: clang.

Fix false positives for -Wnon-c-typedef-for-linkage

Implicit methods for structs can confuse the warning, so exclude checking the 
Decl's that are implicit.  Implicit Decl's for lambdas still need to be 
checked, so skipping all implicit Decl's won't work.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79548

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/anonymous-struct.cpp


Index: clang/test/SemaCXX/anonymous-struct.cpp
===
--- clang/test/SemaCXX/anonymous-struct.cpp
+++ clang/test/SemaCXX/anonymous-struct.cpp
@@ -133,3 +133,20 @@
 int arr[ ? 1 : 2];
   } C; // expected-note {{by this typedef}}
 }
+
+namespace ImplicitDecls {
+  struct Destructor { ~Destructor() {} };
+  typedef struct {} Empty;
+
+  typedef struct {
+Destructor x;
+  } A;
+
+  typedef struct {
+Empty E;
+  } B;
+
+  typedef struct {
+const Empty E;
+  } C;
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -4425,8 +4425,11 @@
 isa(D))
   continue;
 auto *MemberRD = dyn_cast(D);
-if (!MemberRD)
+if (!MemberRD) {
+  if (D->isImplicit())
+continue;
   return {NonCLikeKind::OtherMember, D->getSourceRange()};
+}
 
 //  -- contain a lambda-expression,
 if (MemberRD->isLambda())


Index: clang/test/SemaCXX/anonymous-struct.cpp
===
--- clang/test/SemaCXX/anonymous-struct.cpp
+++ clang/test/SemaCXX/anonymous-struct.cpp
@@ -133,3 +133,20 @@
 int arr[ ? 1 : 2];
   } C; // expected-note {{by this typedef}}
 }
+
+namespace ImplicitDecls {
+  struct Destructor { ~Destructor() {} };
+  typedef struct {} Empty;
+
+  typedef struct {
+Destructor x;
+  } A;
+
+  typedef struct {
+Empty E;
+  } B;
+
+  typedef struct {
+const Empty E;
+  } C;
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -4425,8 +4425,11 @@
 isa(D))
   continue;
 auto *MemberRD = dyn_cast(D);
-if (!MemberRD)
+if (!MemberRD) {
+  if (D->isImplicit())
+continue;
   return {NonCLikeKind::OtherMember, D->getSourceRange()};
+}
 
 //  -- contain a lambda-expression,
 if (MemberRD->isLambda())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73762: New warning for for-loops where the iteration does not match the loop condition

[Richard Trieu via Phabricator via cfe-commits]

rtrieu created this revision.

This is a warning for when the increment/decrement in a for loop does not match 
the condition in the loop.  If the condition has a relational operator, one 
operand can be deduced to be the larger value and the other operand the smaller 
value when the loop is run.  For the loop to terminate, the smaller value needs 
to increase or the larger value needs to decrease, while the opposite will 
cause the loop to not terminate.

Correct:

  for (...; x < y; ++x) {}
  for (...; x < y; --y) {}

Incorrect:

  for (...; x < y; --x) {}
  for (...; x < y; ++y) {}

The warning comes with two attached notes.  One is to flip the direction of the 
comparison (">" to "<", etc) and other is to change the increment decrement 
step ("--" to "++" or vice versa).

An exception is made for unsigned values, since some code uses the unsigned 
overflow/underflow characteristics.


https://reviews.llvm.org/D73762

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaStmt.cpp
  clang/test/SemaCXX/warn-loop-analysis.cpp

Index: clang/test/SemaCXX/warn-loop-analysis.cpp
===
--- clang/test/SemaCXX/warn-loop-analysis.cpp
+++ clang/test/SemaCXX/warn-loop-analysis.cpp
@@ -295,7 +295,69 @@
   for (auto[i, j, k] = arr; a < b; ++a) { }
 
   for (auto [i, j, k] = arr; i < a;) { }
-  for (auto[i, j, k] = arr; i < a; ++a) { }
+  for (auto[i, j, k] = arr; i < a; --a) { }
   for (auto[i, j, k] = arr; i < a; ++i) { }
   for (auto[i, j, k] = arr; i < a; ++arr[0]) { }
 };
+
+void test11(int a, int b, unsigned c, unsigned d) {
+  // Wrong increment
+  for (; a < b; --a) {}
+  // expected-warning@-1 {{for-loop comparison has a less than operator (<) but the loop step decrements the left-hand side of the comparison}}
+  // expected-note@-2 {{flip comparison to greater than}}
+  // expected-note@-3 {{or use increment}}
+  for (; a <= b; a--) {}
+  // expected-warning@-1 {{for-loop comparison has a less than or equal to operator (<=) but the loop step decrements the left-hand side of the comparison}}
+  // expected-note@-2 {{flip comparison to greater than or equal to}}
+  // expected-note@-3 {{or use increment}}
+  for (; a < b; ++b) {}
+  // expected-warning@-1 {{for-loop comparison has a less than operator (<) but the loop step increments the right-hand side of the comparison}}
+  // expected-note@-2 {{flip comparison to greater than}}
+  // expected-note@-3 {{or use decrement}}
+  for (; a <= b; b++) {}
+  // expected-warning@-1 {{for-loop comparison has a less than or equal to operator (<=) but the loop step increments the right-hand side of the comparison}}
+  // expected-note@-2 {{flip comparison to greater than or equal to}}
+  // expected-note@-3 {{or use decrement}}
+  for (; a > b; ++a) {}
+  // expected-warning@-1 {{for-loop comparison has a greater than operator (>) but the loop step increments the left-hand side of the comparison}}
+  // expected-note@-2 {{flip comparison to less than}}
+  // expected-note@-3 {{or use decrement}}
+  for (; a >= b; a++) {}
+  // expected-warning@-1 {{for-loop comparison has a greater than or equal to operator (>=) but the loop step increments the left-hand side of the comparison}}
+  // expected-note@-2 {{flip comparison to less than or equal to}}
+  // expected-note@-3 {{or use decrement}}
+  for (; a > b; --b) {}
+  // expected-warning@-1 {{for-loop comparison has a greater than operator (>) but the loop step decrements the right-hand side of the comparison}}
+  // expected-note@-2 {{flip comparison to less than}}
+  // expected-note@-3 {{or use increment}}
+  for (; a >= b; b--) {}
+  // expected-warning@-1 {{for-loop comparison has a greater than or equal to operator (>=) but the loop step decrements the right-hand side of the comparison}}
+  // expected-note@-2 {{flip comparison to less than or equal to}}
+  // expected-note@-3 {{or use increment}}
+
+  // Correct
+  for (; a < b; ++a) {}
+  for (; a <= b; a++) {}
+  for (; a < b; --b) {}
+  for (; a <= b; b--) {}
+  for (; a > b; --a) {}
+  for (; a >= b; a--) {}
+  for (; a > b; ++b) {}
+  for (; a >= b; b++) {}
+
+  // Unsigned underflow and overlow are well-defined.
+  for (; c < d; ++c) {}
+  for (; c < d; --c) {}
+  for (; c < d; d++) {}
+  for (; c < d; d--) {}
+
+  class vector {
+   public:
+unsigned size() { return 1; };
+  };
+
+  vector v;
+  for (unsigned i = v.size() - 1; i < v.size(); --i) {
+// Intentionally using an underflow to allow i to equal 0.
+  }
+}
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -1739,6 +1739,93 @@
  << LoopIncrement;
   }
 
+  // Emit a warning when the increment is the opposite of what is implied
+  // by the condition.  For instance, the condition 'x < y' implies the correct
+  // increment is either '++x' or '--y' and will generate a warning with
+  

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

I'm in favor of splitting the warning into subgroups, then deciding which ones 
should be in -Wall.  We've done this with other warnings such as the conversion 
warnings and tautological compare warnings, with only a subset of them in -Wall.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73007/new/

https://reviews.llvm.org/D73007



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72552: [Concepts] Constraint Satisfaction Caching

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/AST/ASTConcept.cpp:17
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/SemaConcept.h"
 using namespace clang;

This causes a circular dependency between AST and Sema.  It looks like you are 
including this header to get access to some classes, but you should include the 
direct header instead.  These are the headers for the classes you are using in 
ConstraintSatisfaction::Profile:

NamedDecl -> clang/AST/Decl.h
TemplateArgument -> clang/AST/TemplateBase.h
ArrayRef -> llvm/ADT/ArrayRef.h
FoldingSetNodeID -> llvm/ADT/FoldingSet.h

Please update the header includes to resolve the circular dependency.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72552/new/

https://reviews.llvm.org/D72552



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Heads up in case it affects you refactoring work:
While looking through this code, I found a bug I previously made.  I fixed it 
with a small change, but that lies in the middle of your refactoring during 
FieldDecl handling.  The change is here:

https://reviews.llvm.org/rGa60e8927297005898b10a46300d929ba5cf7833c


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71734/new/

https://reviews.llvm.org/D71734



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72551: Warn when a string literal is used in a range-based for-loop

[Richard Trieu via Phabricator via cfe-commits]

rtrieu created this revision.

String literals used in ranged-based for-loops will actually process the 
terminating null character as part of the loop, which may be unexpected.

  // This runs three times, once for c = 'h', then c = 'i', and finally as c = 
'\0'
  for (char c : "hi") 

This patch adds a warning to -Wrange-loop-analysis when this is used.  Two ways 
to silence the warning are proposed, by either handling the null character in 
the first statement of the loop body or by putting the string literal in 
parentheses.

  // warning
  for (char c : "hi") {}
  
  // silence by handling null character
  for (char c : "hi") {
if (c == '\0') {}
  }
  
  // silence by parentheses
  for (char c : ("hi")) {}


https://reviews.llvm.org/D72551

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaStmt.cpp
  clang/test/SemaCXX/warn-range-loop-analysis.cpp

Index: clang/test/SemaCXX/warn-range-loop-analysis.cpp
===
--- clang/test/SemaCXX/warn-range-loop-analysis.cpp
+++ clang/test/SemaCXX/warn-range-loop-analysis.cpp
@@ -297,3 +297,66 @@
   for (int x : I) {}
   // No warning
 }
+
+namespace StringLiteral {
+void test1(int x) {
+  for (int c : "hello world") {}
+  // expected-warning@-1 {{range-based for-loop working over a string literal will process the trailing null character}}
+  // expected-note@-2 {{fix by explicitly handling the null character at the beginning of the loop body with an if statement}}
+  // expected-note@-3 {{place parenthesis around the string literal to silence this warning}}
+  for (char c : "hello world") {}
+  // expected-warning@-1 {{range-based for-loop working over a string literal will process the trailing null character}}
+  // expected-note@-2 {{fix by explicitly handling the null character at the beginning of the loop body with an if statement}}
+  // expected-note@-3 {{place parenthesis around the string literal to silence this warning}}
+
+  // Various methods of silencing the warning.
+  for (char c : ("hello world")) {}
+  for (char c : "hello world") { if (c > 0) {} }
+  for (char c : "hello world") { if (c == '\0') {} }
+  for (char c : "hello world") { if (c != 0) {} }
+  for (char c : "hello world") { if (c) {} }
+  for (char c : "hello world") if (c > 0) {}
+  for (char c : "hello world") if (c == '\0') {}
+  for (char c : "hello world") if (c != 0) {}
+  for (char c : "hello world") if (c) {}
+
+  // Almost silencing the warning.
+  for (char c : "hello world") { if (c >> 0) {} }
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+  for (char c : "hello world") { if (x > 0) {} }
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+  for (char c : "hello world") { if (x == '\0') {} }
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+  for (char c : "hello world") { if (x != 0) {} }
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+  for (char c : "hello world") { if (x) {} }
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+  for (char c : "hello world") if (x > 0) {}
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+  for (char c : "hello world") if (x == '\0') {}
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+  for (char c : "hello world") if (x != 0) {}
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+  for (char c : "hello world") if (x) {}
+  // expected-warning@-1 {{null character}}
+  // expected-note@-2 {{if statement}}
+  // expected-note@-3 {{silence this warning}}
+}
+
+} // namespace StringLiteral
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -2851,6 +2851,102 @@
   }
 }
 
+// Diagnoses when a string literal is used as the range for a range-based
+// for-loop, because the trailing null will unexpected be handled with the
+// rest of the characters in the string.  The warning can be silenced either
+// by add parentheses around the string literal or by explicitly handling
+// the null character with an if statement at the beginning of the loop body.
+static void DiagnoseForRangeStringLiteral(Sema ,
+  const CXXForRangeStmt *ForStmt) {
+  if (SemaRef.Diags.isIgnored(diag::warn_for_range_string_literal,
+  

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:11007
 
+  // Issue any pending ODR-failure diagnostics.
+  for (auto  : RecordOdrMergeFailures) {

bruno wrote:
> rtrieu wrote:
> > Is this just a copy of the other loop?  That's a lot of code duplication.
> There's a lot of copy but a lot of CXXRecord specific logic got trimmed out 
> and there are small differences in the approach, so a direct refactor of it 
> won't work, but there's certainly more small pieces that can be factored out, 
> will follow up with that.
I like this refactoring much better.  Probably more refactoring could be done 
to this function since it's so long, but that's an issue for another time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71734/new/

https://reviews.llvm.org/D71734



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:11007
 
+  // Issue any pending ODR-failure diagnostics.
+  for (auto  : RecordOdrMergeFailures) {

Is this just a copy of the other loop?  That's a lot of code duplication.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71734/new/

https://reviews.llvm.org/D71734



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:2812-2813
 
-  // TODO: Determine a maximum size that a POD type can be before a diagnostic
-  // should be emitted.  Also, only ignore POD types with trivial copy
-  // constructors.
-  if (VariableType.isPODType(SemaRef.Context))
+  // Small trivially copyable types are cheap to copy. Do not emit the
+  // diagnostic for these instances.
+  ASTContext  = SemaRef.Context;

You should add why 64 bytes was chosen to this comment to both explain the 64 * 
8 magic number in the following lines, and to let people reading this code know 
 why that was picked without needed to look up the patch notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71503: New warning on for-loops that never run because condition is false on the first iteration

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 234180.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71503/new/

https://reviews.llvm.org/D71503

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/SemaCXX/warn-loop-analysis-first-condition.cpp

Index: clang/test/SemaCXX/warn-loop-analysis-first-condition.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-loop-analysis-first-condition.cpp
@@ -0,0 +1,202 @@
+// RUN: %clang_cc1 %s -Wfor-loop-analysis -verify
+// RUN: %clang_cc1 %s -Wall -verify
+
+#define OneHundred 100
+
+void test() {
+  // Loop variable
+  for (int i = OneHundred; i < 50; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 100 on loop initialization, but comparing 100 to 50 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = OneHundred; i > 50; --i) {}
+
+  // Outside variable
+  int j;
+  for (j = OneHundred; j < 50; ++j) {}
+  // expected-warning@-1{{variable 'j' is set to value 100 on loop initialization, but comparing 100 to 50 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (j = OneHundred; j > 50; --j) {}
+
+  // int->float conversion
+  for (int i = OneHundred; i < 5e1; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 100 on loop initialization, but comparing 100 to 5.00e+01 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = OneHundred; i > 5e1; --i) {}
+
+  // const values
+  const int kStart = 0;
+  const int kInterval = 10;
+  for (int i = kStart; i < kStart; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 0 on loop initialization, but comparing 0 to 0 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = kStart; i < kStart + kInterval; ++i) {}
+
+  // Reversed comparison operator
+  for (int i = 100; i <= 1; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 100 on loop initialization, but comparing 100 to 1 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = 100; i >= 1; --i) {}
+
+  // Range endpoints mistakes
+  const int kMin = 0;
+  const int kMax = 8;
+  for (int i = kMin; i < kMin; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 0 on loop initialization, but comparing 0 to 0 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = kMax; i < kMin; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 8 on loop initialization, but comparing 8 to 0 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = kMin; i < kMax; ++i) {}
+
+  for (int i = 16; i < 16; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 16 on loop initialization, but comparing 16 to 16 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = 8; i < 16; ++i) {}
+
+  for (int i = 1; i <= (1 >> 5); ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 1 on loop initialization, but comparing 1 to 0 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = 1; i <= (1 << 5); ++i) {}
+
+  // Complex comparion operand
+  const int TwoK = 1024 * 2;
+  for (int i = 2048; i < TwoK; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 2048 on loop initialization, but comparing 2048 to 2048 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (int i = 1024; i < TwoK; ++i) {}
+
+  // Signedness mistake
+  for (unsigned i = -5; i < 5; ++i) {}
+  // expected-warning@-1{{variable 'i' is set to value 4294967291 on loop initialization, but comparing 4294967291 to 5 in the loop condition is false and the loop body will never run}}
+  // expected-note@-2{{place parentheses around the condition to silence this warning}}
+  for (signed i = -5; i < 5; ++i) {}
+
+  for (int i = 0x; i > 10; i -= 10) {}
+  // expected-warning@-1{{variable 'i' is set to value -1 on loop initialization, but comparing 

[PATCH] D71503: New warning on for-loops that never run because condition is false on the first iteration

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D71503#1784697 , @lebedev.ri wrote:

> Thank you for working on this!
>  This seems to be missing tests.


My bad, still getting used to git.  Updated with a test now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71503/new/

https://reviews.llvm.org/D71503



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71503: New warning on for-loops that never run because condition is false on the first iteration

[Richard Trieu via Phabricator via cfe-commits]

rtrieu created this revision.

This is a proposed warning to be added to -Wfor-loop-analysis, which is part of 
-Wall.  This warning will catch instances where the condition will be false on 
the first iteration and the loop body will never be run.  The warning will be 
emitted when:

1. The for-loop initializes or sets the value of a single variable to a 
constant value
2. The condition is a simple comparison between the variable and another 
constant value
3. If the initial value of the variable substituted into the comparison would 
cause the comparison to be false

In order to make step 3 work, the constant value from step 1 may need to be 
casted to a different type.  The casts can be extracted from the comparison 
operand.  This allows the warning to work with both integer and floating point 
types, as well as mixing types.

Two exceptions to this warning.

1. Parentheses around the condition will silence this warning.  This is 
suggested in a note.
2. If the variable is used as an array index in the body, and the condition is 
less than a value the same as the array size.

Example:
https://reviews.llvm.org/D48498
This warning would have caught the issue that lebedev.ri brought up about the 
loop not running.


https://reviews.llvm.org/D71503

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaStmt.cpp

Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/RecursiveASTVisitor.h"
@@ -1739,6 +1740,258 @@
  << LoopIncrement;
   }
 
+  // Look for any array access of the form array[x] where 'x' is a given Decl
+  // and array is constant array with given size.
+  class ArrayFinder : public ConstEvaluatedExprVisitor {
+// Tracking boolean
+bool FoundArray;
+// Decl used as array index
+const VarDecl *VD;
+// Target size for array
+llvm::APInt Size;
+
+  public:
+typedef ConstEvaluatedExprVisitor Inherited;
+ArrayFinder(const ASTContext ) : Inherited(Context) { }
+
+bool CheckForArrays(const Stmt *Body, llvm::APInt TargetArraySize,
+const VarDecl *D) {
+  FoundArray = false;
+  Size = TargetArraySize;
+  VD = D;
+  Visit(Body);
+  return FoundArray;
+}
+
+void VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {
+  if (CheckArrayType(E->getBase()) && CheckIndex(E->getIdx())) {
+FoundArray = true;
+  }
+  Inherited::VisitArraySubscriptExpr(E);
+}
+
+  private:
+// Helper function for checking array access.
+bool CheckArrayType(const Expr *E) {
+  QualType ArrayTy = E->IgnoreImpCasts()->getType();
+  if (!ArrayTy->isConstantArrayType())
+return false;
+  const ConstantArrayType *CAT = Context.getAsConstantArrayType(ArrayTy);
+  if (!CAT)
+return false;
+  if (!llvm::APInt::isSameValue(CAT->getSize(), Size))
+return false;
+  return true;
+}
+
+// Helper function for checking array access.
+bool CheckIndex(const Expr *E) {
+  const DeclRefExpr *DRE = dyn_cast(E->IgnoreImpCasts());
+  return DRE && DRE->getDecl() == VD;
+}
+
+  };
+
+  // Emit a warning when the condition of a for-loop is false on the first
+  // iteration, making the loop body never executed.  Conditions in parenthesis
+  // will silence the warning.  Loop variables used as array accesses are also
+  // skipped.
+  void CheckFirstIterationCondition(Sema , const Stmt *First,
+const Expr *Second, const Stmt *Body) {
+if (!First || !Second || Second->isTypeDependent() ||
+Second->isValueDependent())
+  return;
+if (S.inTemplateInstantiation())
+  return;
+if (Second->getExprLoc().isMacroID())
+  return;
+
+if (S.Diags.isIgnored(diag::warn_loop_never_run, Second->getBeginLoc()))
+  return;
+
+// Only work on loop conditions that are comparisons.
+const BinaryOperator *CompareBO = dyn_cast(Second);
+if (!CompareBO || !CompareBO->isComparisonOp())
+  return;
+
+// Examine the loop initilization and extract the single Decl used and its
+// initial value.
+const VarDecl *D = nullptr;
+APValue InitValue;
+if (const DeclStmt *DS = dyn_cast(First)) {
+  // A loop variable being declared.
+  if (!DS->isSingleDecl())
+return;
+  D = dyn_cast(DS->getSingleDecl());
+  if (!D || !D->getInit())
+return;
+  Expr::EvalResult Result;
+  if (!D->getInit()->EvaluateAsRValue(Result, S.Context))
+return;
+  InitValue = Result.Val;
+} else if (const Expr 

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D69292#1747062 , @mclow.lists wrote:

> Sorry I'm late to the party; I've been traveling for 3+ weeks.
>  I would like to be reassured that the following code will not warn:
>
>   `
>long foo = ...; // some calculation
>if (foo < std::numeric_limits::min() || foo > 
> std::numeric_limits::max()) .
>   
>
> This is important for systems where `sizeof(int) == sizeof(long)`


The question is whether there's a warning when a value is tautologically 
compared with the min or max of that value's type.  They fall under 
-Wtype-limits or -Wtautological-constant-in-range-compare.  Despite the 
"tautological" in the name, they do not fall under -Wtautological-compare.  
Therefore, your example code will not get any warnings with -Wall.

I checked the clang tests and this covered by 
test/Sema/tautological-constant-compare.c, testing both the warnings is active 
only when the specific warning groups are active (the #ifdef TEST sections) and 
that the warnings are off for no warning flags, just -Wextra, and just -Wall.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69292/new/

https://reviews.llvm.org/D69292



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

[Richard Trieu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9740f9f0b6e5: Add -Wtautological-compare to -Wall (authored 
by rtrieu).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D69292?vs=228361=228970#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69292/new/

https://reviews.llvm.org/D69292

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-bitwise-compare.c
  clang/test/Sema/warn-overlap.c
  clang/test/SemaCXX/warn-bitwise-compare.cpp

Index: clang/test/SemaCXX/warn-bitwise-compare.cpp
===
--- clang/test/SemaCXX/warn-bitwise-compare.cpp
+++ clang/test/SemaCXX/warn-bitwise-compare.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 void test(int x) {
   bool b1 = (8 & x) == 3;
Index: clang/test/Sema/warn-overlap.c
===
--- clang/test/Sema/warn-overlap.c
+++ clang/test/Sema/warn-overlap.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-overlap-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused -Wno-loop-analysis %s
 
 #define mydefine 2
 
Index: clang/test/Sema/warn-bitwise-compare.c
===
--- clang/test/Sema/warn-bitwise-compare.c
+++ clang/test/Sema/warn-bitwise-compare.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 #define mydefine 2
 
Index: clang/test/Misc/warning-wall.c
===
--- /dev/null
+++ clang/test/Misc/warning-wall.c
@@ -0,0 +1,95 @@
+RUN: diagtool tree -Wall > %t 2>&1
+RUN: FileCheck --input-file=%t %s
+
+ CHECK:-Wall
+CHECK-NEXT:  -Wmost
+CHECK-NEXT:-Wchar-subscripts
+CHECK-NEXT:-Wcomment
+CHECK-NEXT:-Wdelete-non-virtual-dtor
+CHECK-NEXT:  -Wdelete-non-abstract-non-virtual-dtor
+CHECK-NEXT:  -Wdelete-abstract-non-virtual-dtor
+CHECK-NEXT:-Wfor-loop-analysis
+CHECK-NEXT:-Wformat
+CHECK-NEXT:  -Wformat-extra-args
+CHECK-NEXT:  -Wformat-zero-length
+CHECK-NEXT:  -Wnonnull
+CHECK-NEXT:  -Wformat-security
+CHECK-NEXT:  -Wformat-y2k
+CHECK-NEXT:  -Wformat-invalid-specifier
+CHECK-NEXT:-Wimplicit
+CHECK-NEXT:  -Wimplicit-function-declaration
+CHECK-NEXT:  -Wimplicit-int
+CHECK-NEXT:-Winfinite-recursion
+CHECK-NEXT:-Wint-in-bool-context
+CHECK-NEXT:-Wmismatched-tags
+CHECK-NEXT:-Wmissing-braces
+CHECK-NEXT:-Wmove
+CHECK-NEXT:  -Wpessimizing-move
+CHECK-NEXT:  -Wredundant-move
+CHECK-NEXT:  -Wreturn-std-move
+CHECK-NEXT:  -Wself-move
+CHECK-NEXT:-Wmultichar
+CHECK-NEXT:-Wreorder
+CHECK-NEXT:  -Wreorder-ctor
+CHECK-NEXT:  -Wreorder-init-list
+CHECK-NEXT:-Wreturn-type
+CHECK-NEXT:  -Wreturn-type-c-linkage
+CHECK-NEXT:-Wself-assign
+CHECK-NEXT:  -Wself-assign-overloaded
+CHECK-NEXT:  -Wself-assign-field
+CHECK-NEXT:-Wself-move
+CHECK-NEXT:-Wsizeof-array-argument
+CHECK-NEXT:-Wsizeof-array-decay
+CHECK-NEXT:-Wstring-plus-int
+CHECK-NEXT:-Wtautological-compare
+CHECK-NEXT:  -Wtautological-constant-compare
+CHECK-NEXT:-Wtautological-constant-out-of-range-compare
+CHECK-NEXT:  -Wtautological-pointer-compare
+CHECK-NEXT:  -Wtautological-overlap-compare
+CHECK-NEXT:  -Wtautological-bitwise-compare
+CHECK-NEXT:  -Wtautological-undefined-compare
+CHECK-NEXT:  -Wtautological-objc-bool-compare
+CHECK-NEXT:-Wtrigraphs
+CHECK-NEXT:-Wuninitialized
+CHECK-NEXT:  -Wsometimes-uninitialized
+CHECK-NEXT:  -Wstatic-self-init
+CHECK-NEXT:-Wunknown-pragmas
+CHECK-NEXT:-Wunused
+CHECK-NEXT:  -Wunused-argument
+CHECK-NEXT:  -Wunused-function
+CHECK-NEXT:-Wunneeded-internal-declaration
+CHECK-NEXT:  -Wunused-label
+CHECK-NEXT:  -Wunused-private-field
+CHECK-NEXT:  -Wunused-lambda-capture
+CHECK-NEXT:  -Wunused-local-typedef
+CHECK-NEXT:  -Wunused-value
+CHECK-NEXT:-Wunused-comparison
+CHECK-NEXT:-Wunused-result
+CHECK-NEXT:-Wunevaluated-expression
+CHECK-NEXT:  -Wpotentially-evaluated-expression
+CHECK-NEXT:  -Wunused-variable
+CHECK-NEXT:-Wunused-const-variable
+CHECK-NEXT:  -Wunused-property-ivar
+CHECK-NEXT:-Wvolatile-register-var
+CHECK-NEXT:-Wobjc-missing-super-calls
+CHECK-NEXT:-Wobjc-designated-initializers
+CHECK-NEXT:-Wobjc-flexible-array
+CHECK-NEXT:-Woverloaded-virtual
+CHECK-NEXT:-Wprivate-extern
+CHECK-NEXT:-Wcast-of-sel-type
+CHECK-NEXT:-Wextern-c-compat
+CHECK-NEXT:-Wuser-defined-warnings

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 228361.
rtrieu added a comment.

Add -Wall tests to check that certain warning groups are active with it and a 
test to check all warning groups that are turned on by -Wall.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69292/new/

https://reviews.llvm.org/D69292

Files:
  include/clang/Basic/DiagnosticGroups.td
  test/Misc/warning-wall.c
  test/Sema/warn-bitwise-compare.c
  test/Sema/warn-overlap.c
  test/SemaCXX/warn-bitwise-compare.cpp

Index: test/SemaCXX/warn-bitwise-compare.cpp
===
--- test/SemaCXX/warn-bitwise-compare.cpp
+++ test/SemaCXX/warn-bitwise-compare.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 void test(int x) {
   bool b1 = (8 & x) == 3;
Index: test/Sema/warn-overlap.c
===
--- test/Sema/warn-overlap.c
+++ test/Sema/warn-overlap.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-overlap-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused -Wno-loop-analysis %s
 
 #define mydefine 2
 
Index: test/Sema/warn-bitwise-compare.c
===
--- test/Sema/warn-bitwise-compare.c
+++ test/Sema/warn-bitwise-compare.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 #define mydefine 2
 
Index: test/Misc/warning-wall.c
===
--- test/Misc/warning-wall.c
+++ test/Misc/warning-wall.c
@@ -0,0 +1,95 @@
+RUN: diagtool tree -Wall > %t 2>&1
+RUN: FileCheck --input-file=%t %s
+
+ CHECK:-Wall
+CHECK-NEXT:  -Wmost
+CHECK-NEXT:-Wchar-subscripts
+CHECK-NEXT:-Wcomment
+CHECK-NEXT:-Wdelete-non-virtual-dtor
+CHECK-NEXT:  -Wdelete-non-abstract-non-virtual-dtor
+CHECK-NEXT:  -Wdelete-abstract-non-virtual-dtor
+CHECK-NEXT:-Wfor-loop-analysis
+CHECK-NEXT:-Wformat
+CHECK-NEXT:  -Wformat-extra-args
+CHECK-NEXT:  -Wformat-zero-length
+CHECK-NEXT:  -Wnonnull
+CHECK-NEXT:  -Wformat-security
+CHECK-NEXT:  -Wformat-y2k
+CHECK-NEXT:  -Wformat-invalid-specifier
+CHECK-NEXT:-Wimplicit
+CHECK-NEXT:  -Wimplicit-function-declaration
+CHECK-NEXT:  -Wimplicit-int
+CHECK-NEXT:-Winfinite-recursion
+CHECK-NEXT:-Wint-in-bool-context
+CHECK-NEXT:-Wmismatched-tags
+CHECK-NEXT:-Wmissing-braces
+CHECK-NEXT:-Wmove
+CHECK-NEXT:  -Wpessimizing-move
+CHECK-NEXT:  -Wredundant-move
+CHECK-NEXT:  -Wreturn-std-move
+CHECK-NEXT:  -Wself-move
+CHECK-NEXT:-Wmultichar
+CHECK-NEXT:-Wreorder
+CHECK-NEXT:  -Wreorder-ctor
+CHECK-NEXT:  -Wreorder-init-list
+CHECK-NEXT:-Wreturn-type
+CHECK-NEXT:  -Wreturn-type-c-linkage
+CHECK-NEXT:-Wself-assign
+CHECK-NEXT:  -Wself-assign-overloaded
+CHECK-NEXT:  -Wself-assign-field
+CHECK-NEXT:-Wself-move
+CHECK-NEXT:-Wsizeof-array-argument
+CHECK-NEXT:-Wsizeof-array-decay
+CHECK-NEXT:-Wstring-plus-int
+CHECK-NEXT:-Wtautological-compare
+CHECK-NEXT:  -Wtautological-constant-compare
+CHECK-NEXT:-Wtautological-constant-out-of-range-compare
+CHECK-NEXT:  -Wtautological-pointer-compare
+CHECK-NEXT:  -Wtautological-overlap-compare
+CHECK-NEXT:  -Wtautological-bitwise-compare
+CHECK-NEXT:  -Wtautological-undefined-compare
+CHECK-NEXT:  -Wtautological-objc-bool-compare
+CHECK-NEXT:-Wtrigraphs
+CHECK-NEXT:-Wuninitialized
+CHECK-NEXT:  -Wsometimes-uninitialized
+CHECK-NEXT:  -Wstatic-self-init
+CHECK-NEXT:-Wunknown-pragmas
+CHECK-NEXT:-Wunused
+CHECK-NEXT:  -Wunused-argument
+CHECK-NEXT:  -Wunused-function
+CHECK-NEXT:-Wunneeded-internal-declaration
+CHECK-NEXT:  -Wunused-label
+CHECK-NEXT:  -Wunused-private-field
+CHECK-NEXT:  -Wunused-lambda-capture
+CHECK-NEXT:  -Wunused-local-typedef
+CHECK-NEXT:  -Wunused-value
+CHECK-NEXT:-Wunused-comparison
+CHECK-NEXT:-Wunused-result
+CHECK-NEXT:-Wunevaluated-expression
+CHECK-NEXT:  -Wpotentially-evaluated-expression
+CHECK-NEXT:  -Wunused-variable
+CHECK-NEXT:-Wunused-const-variable
+CHECK-NEXT:  -Wunused-property-ivar
+CHECK-NEXT:-Wvolatile-register-var
+CHECK-NEXT:-Wobjc-missing-super-calls
+CHECK-NEXT:-Wobjc-designated-initializers
+CHECK-NEXT:-Wobjc-flexible-array
+CHECK-NEXT:-Woverloaded-virtual
+CHECK-NEXT:-Wprivate-extern
+CHECK-NEXT:-Wcast-of-sel-type
+CHECK-NEXT:-Wextern-c-compat
+CHECK-NEXT:-Wuser-defined-warnings
+CHECK-NEXT:  -Wparentheses
+CHECK-NEXT:-Wlogical-op-parentheses
+CHECK-NEXT:-Wlogical-not-parentheses
+CHECK-NEXT:-Wbitwise-conditional-parentheses
+CHECK-NEXT:

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D69292#1718415 , @thakis wrote:

> Abstractly this lgtm. Do you have any data on true / false positive rates?


Looking at the various warnings in -Wtautological-compare, only 
-Wtautological-overlap-compare and -Wtautological-bitwise-compare are not on by 
default.  During my last try with those, they were very high value, and had low 
or no false posivites.

@xbolva00 linked to someone testing out -Wtautological-compare.  The results 
don't show any warnings from the bitwise or overlap warnings.

In D69292#1719093 , @aaron.ballman 
wrote:

> I agree with the changes and want to see this go in, but it needs a test case.


Would this be better if we had a specific test to verify all the warning groups 
in -Wall?  I could see something using diagtool to list and check all the 
warnings.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69292/new/

https://reviews.llvm.org/D69292



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D66046#1717229 , @thakis wrote:

> Mr Trieu, what do you think about adding some or all of the 
> Wtautological-compare warnings to Wall


I thought -Wtautological-compare was already part of -Wall, so I'm all for 
adding it.  I made a patch https://reviews.llvm.org/D69292 with the proposed 
move to -Wall so it will have some more visibility.   I'll be out for the 
conference and can commit it afterwards if no issues are brought up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66046/new/

https://reviews.llvm.org/D66046



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

[Richard Trieu via Phabricator via cfe-commits]

rtrieu created this revision.

Per https://reviews.llvm.org/D66046, patch to move -Wtautological-compare to 
-Wmost, which also adds it to -Wall.  Some warnings in -Wtautological-compare 
and its diagnostic sub-groups are DefaultIgnore, so making them visible in 
-Wall will make them more discoverable overall.


https://reviews.llvm.org/D69292

Files:
  include/clang/Basic/DiagnosticGroups.td


Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -842,6 +842,7 @@
 SizeofArrayArgument,
 SizeofArrayDecay,
 StringPlusInt,
+TautologicalCompare,
 Trigraphs,
 Uninitialized,
 UnknownPragmas,


Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -842,6 +842,7 @@
 SizeofArrayArgument,
 SizeofArrayDecay,
 StringPlusInt,
+TautologicalCompare,
 Trigraphs,
 Uninitialized,
 UnknownPragmas,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG637af4cc3780: Add -Wbitwise-conditional-parentheses to warn 
on mixing | and  with ?: (authored by 
rtrieu).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D66043?vs=219213=225732#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66043/new/

https://reviews.llvm.org/D66043

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/parentheses.c

Index: clang/test/Sema/parentheses.c
===
--- clang/test/Sema/parentheses.c
+++ clang/test/Sema/parentheses.c
@@ -93,6 +93,28 @@
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
+
+  (void)(b ? 0xf0 : 0x10 | b ? 0x5 : 0x2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}
+
+  (void)((b ? 0xf0 : 0x10) | (b ? 0x5 : 0x2));  // no warning, has parentheses
+  (void)(b ? 0xf0 : (0x10 | b) ? 0x5 : 0x2);  // no warning, has parentheses
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '&'}} expected-note 2{{place parentheses}}
+
+  (void)((x | b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x | (b ? 1 : 2));  // no warning, has parentheses
+  (void)((x & b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x & (b ? 1 : 2));  // no warning, has parentheses
+
+  // Only warn on uses of the bitwise operators, and not the logical operators.
+  // The bitwise operators are more likely to be bugs while the logical
+  // operators are more likely to be used correctly.  Since there is no
+  // explicit logical-xor operator, the bitwise-xor is commonly used instead.
+  // For this warning, treat the bitwise-xor as if it were a logical operator.
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator
+  (void)(x && b ? 1 : 2);  // no warning, logical operator
 }
 
 // RUN: not %clang_cc1 -fsyntax-only -Wparentheses -Werror -fdiagnostics-show-option %s 2>&1 | FileCheck %s -check-prefix=CHECK-FLAG
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -7620,7 +7620,12 @@
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return BinaryOperator::isAdditiveOp(Opc) ||
  BinaryOperator::isMultiplicativeOp(Opc) ||
- BinaryOperator::isShiftOp(Opc);
+ BinaryOperator::isShiftOp(Opc) || Opc == BO_And || Opc == BO_Or;
+  // This only checks for bitwise-or and bitwise-and, but not bitwise-xor and
+  // not any of the logical operators.  Bitwise-xor is commonly used as a
+  // logical-xor because there is no logical-xor operator.  The logical
+  // operators, including uses of xor, have a high false positive rate for
+  // precedence warnings.
 }
 
 /// IsArithmeticBinaryExpr - Returns true if E is an arithmetic binary
@@ -7710,7 +7715,11 @@
   // The condition is an arithmetic binary expression, with a right-
   // hand side that looks boolean, so warn.
 
-  Self.Diag(OpLoc, diag::warn_precedence_conditional)
+  unsigned DiagID = BinaryOperator::isBitwiseOp(CondOpcode)
+? diag::warn_precedence_bitwise_conditional
+: diag::warn_precedence_conditional;
+
+  Self.Diag(OpLoc, DiagID)
   << Condition->getSourceRange()
   << BinaryOperator::getOpcodeStr(CondOpcode);
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5745,6 +5745,9 @@
 def warn_precedence_conditional : Warning<
   "operator '?:' has lower precedence than '%0'; '%0' will be evaluated first">,
   InGroup;
+def warn_precedence_bitwise_conditional : Warning<
+  "operator '?:' has lower precedence than '%0'; '%0' will be evaluated first">,
+  InGroup;
 def note_precedence_conditional_first : Note<
   "place parentheses around the '?:' expression to evaluate it first">;
 
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -296,6 +296,7 @@
 def FlexibleArrayExtensions : DiagGroup<"flexible-array-extensions">;
 def FourByteMultiChar : DiagGroup<"four-char-constants">;
 def GlobalConstructors : 

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b0d14a8f0cc: New tautological warning for bitwise-or with 
non-zero constant always true. (authored by rtrieu).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D66046?vs=214733=225726#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66046/new/

https://reviews.llvm.org/D66046

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Analysis/CFG.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Sema/warn-bitwise-compare.c
  clang/test/SemaCXX/warn-bitwise-compare.cpp

Index: clang/test/SemaCXX/warn-bitwise-compare.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-bitwise-compare.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+
+void test(int x) {
+  bool b1 = (8 & x) == 3;
+  // expected-warning@-1 {{bitwise comparison always evaluates to false}}
+  bool b2 = x | 5;
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b3 = (x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b4 = !!(x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+}
Index: clang/test/Sema/warn-bitwise-compare.c
===
--- clang/test/Sema/warn-bitwise-compare.c
+++ clang/test/Sema/warn-bitwise-compare.c
@@ -1,7 +1,12 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 
 #define mydefine 2
 
+enum {
+  ZERO,
+  ONE,
+};
+
 void f(int x) {
   if ((8 & x) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x & 8) == 4) {}  // expected-warning {{bitwise comparison always evaluates to false}}
@@ -13,6 +18,9 @@
   if ((x & 0x15) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
   if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
 
+  if (!!((8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  int y = ((8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((x & 8) == 8) {}
   if ((x & 8) != 8) {}
   if ((x | 4) == 4) {}
@@ -26,3 +34,14 @@
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
 }
+
+void g(int x) {
+  if (x | 5) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (5 | x) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (!((x | 5))) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+
+  if (x | -1) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (x | ONE) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+
+  if (x | ZERO) {}
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -159,6 +159,20 @@
 S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_always)
 << DiagRange << isAlwaysTrue;
   }
+
+  void compareBitwiseOr(const BinaryOperator *B) override {
+if (HasMacroID(B))
+  return;
+
+SourceRange DiagRange = B->getSourceRange();
+S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_or) << DiagRange;
+  }
+
+  static bool hasActiveDiagnostics(DiagnosticsEngine ,
+   SourceLocation Loc) {
+return !Diags.isIgnored(diag::warn_tautological_overlap_comparison, Loc) ||
+   !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc);
+  }
 };
 } // anonymous namespace
 
@@ -2070,10 +2084,9 @@
   .setAlwaysAdd(Stmt::AttributedStmtClass);
   }
 
-  // Install the logical handler for -Wtautological-overlap-compare
+  // Install the logical handler.
   llvm::Optional LEH;
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-   D->getBeginLoc())) {
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
 LEH.emplace(S);
 AC.getCFGBuildOptions().Observer = &*LEH;
   }
@@ -,9 +2235,8 @@
 checkThrowInNonThrowingFunc(S, FD, AC);
 
   // If none of the previous checks caused a CFG build, trigger one here
-  // for -Wtautological-overlap-compare
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-   D->getBeginLoc())) {
+  // for the logical error handler.
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, 

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In D66046#1696785 , @xbolva00 wrote:

> Could this patch solve https://bugs.llvm.org/show_bug.cgi?id=43573?


No, but I left some notes on the bug on why negative values are hard and where 
to fix it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66046/new/

https://reviews.llvm.org/D66046



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372453: Merge and improve code that detects same value in 
comparisons. (authored by rtrieu, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66045?vs=215002=221152#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66045/new/

https://reviews.llvm.org/D66045

Files:
  cfe/trunk/docs/ReleaseNotes.rst
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/Analysis/CFG.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/Analysis/array-struct-region.cpp
  cfe/trunk/test/Sema/warn-overlap.c
  cfe/trunk/test/SemaCXX/compare-cxx2a.cpp
  cfe/trunk/test/SemaCXX/self-comparison.cpp

Index: cfe/trunk/test/Analysis/array-struct-region.cpp
===
--- cfe/trunk/test/Analysis/array-struct-region.cpp
+++ cfe/trunk/test/Analysis/array-struct-region.cpp
@@ -1,20 +1,26 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN:-Wno-tautological-compare\
 // RUN:-x c %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN:-Wno-tautological-compare\
 // RUN:-x c++ -std=c++14 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN:-Wno-tautological-compare\
 // RUN:-x c++ -std=c++17 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN:-Wno-tautological-compare\
 // RUN:-DINLINE -x c %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN:-Wno-tautological-compare\
 // RUN:-DINLINE -x c++ -std=c++14 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN:-Wno-tautological-compare\
 // RUN:-DINLINE -x c++ -std=c++17 %s
 
 void clang_analyzer_eval(int);
Index: cfe/trunk/test/SemaCXX/self-comparison.cpp
===
--- cfe/trunk/test/SemaCXX/self-comparison.cpp
+++ cfe/trunk/test/SemaCXX/self-comparison.cpp
@@ -40,3 +40,84 @@
 Y::n == Y::n;
 }
 template bool g(); // should not produce any warnings
+
+namespace member_tests {
+struct B {
+  int field;
+  static int static_field;
+  int test(B b) {
+return field == field;  // expected-warning {{self-comparison always evaluates to true}}
+return static_field == static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return static_field == b.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return B::static_field == this->static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return this == this;  // expected-warning {{self-comparison always evaluates to true}}
+
+return field == b.field;
+return this->field == b.field;
+  }
+};
+
+enum {
+  I0,
+  I1,
+  I2,
+};
+
+struct S {
+  int field;
+  static int static_field;
+  int array[4];
+};
+
+struct T {
+  int field;
+  static int static_field;
+  int array[4];
+  S s;
+};
+
+int struct_test(S s1, S s2, S *s3, T t) {
+  return s1.field == s1.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s2.field == s2.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.static_field == s2.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return S::static_field == s1.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->field == s3->field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[0] == s1.array[0];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[I1] == s1.array[I1];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[s2.array[0]] == s1.array[s2.array[0]];  // expected-warning {{self-comparison always evaluates to true}}
+  

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

[Richard Trieu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372448: Improve -Wtautological-overlap-compare (authored by 
rtrieu, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66044?vs=215021=221151#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66044/new/

https://reviews.llvm.org/D66044

Files:
  cfe/trunk/docs/ReleaseNotes.rst
  cfe/trunk/lib/Analysis/CFG.cpp
  cfe/trunk/lib/Analysis/ReachableCode.cpp
  cfe/trunk/test/Analysis/cfg.cpp
  cfe/trunk/test/Sema/warn-overlap.c
  cfe/trunk/test/Sema/warn-unreachable.c
  cfe/trunk/test/SemaCXX/warn-unreachable.cpp

Index: cfe/trunk/lib/Analysis/ReachableCode.cpp
===
--- cfe/trunk/lib/Analysis/ReachableCode.cpp
+++ cfe/trunk/lib/Analysis/ReachableCode.cpp
@@ -247,7 +247,7 @@
 }
 case Stmt::UnaryOperatorClass: {
   const UnaryOperator *UO = cast(S);
-  if (UO->getOpcode() != UO_LNot)
+  if (UO->getOpcode() != UO_LNot && UO->getOpcode() != UO_Minus)
 return false;
   bool SilenceableCondValNotSet =
   SilenceableCondVal && SilenceableCondVal->getBegin().isInvalid();
Index: cfe/trunk/lib/Analysis/CFG.cpp
===
--- cfe/trunk/lib/Analysis/CFG.cpp
+++ cfe/trunk/lib/Analysis/CFG.cpp
@@ -70,11 +70,35 @@
   return D->getLocation();
 }
 
+/// Returns true on constant values based around a single IntegerLiteral.
+/// Allow for use of parentheses, integer casts, and negative signs.
+static bool IsIntegerLiteralConstantExpr(const Expr *E) {
+  // Allow parentheses
+  E = E->IgnoreParens();
+
+  // Allow conversions to different integer kind.
+  if (const auto *CE = dyn_cast(E)) {
+if (CE->getCastKind() != CK_IntegralCast)
+  return false;
+E = CE->getSubExpr();
+  }
+
+  // Allow negative numbers.
+  if (const auto *UO = dyn_cast(E)) {
+if (UO->getOpcode() != UO_Minus)
+  return false;
+E = UO->getSubExpr();
+  }
+
+  return isa(E);
+}
+
 /// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
-/// or EnumConstantDecl from the given Expr. If it fails, returns nullptr.
+/// constant expression or EnumConstantDecl from the given Expr. If it fails,
+/// returns nullptr.
 static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
   E = E->IgnoreParens();
-  if (isa(E))
+  if (IsIntegerLiteralConstantExpr(E))
 return E;
   if (auto *DR = dyn_cast(E->IgnoreParenImpCasts()))
 return isa(DR->getDecl()) ? DR : nullptr;
@@ -121,11 +145,11 @@
 static bool areExprTypesCompatible(const Expr *E1, const Expr *E2) {
   // User intent isn't clear if they're mixing int literals with enum
   // constants.
-  if (isa(E1) != isa(E2))
+  if (isa(E1) != isa(E2))
 return false;
 
   // Integer literal comparisons, regardless of literal type, are acceptable.
-  if (isa(E1))
+  if (!isa(E1))
 return true;
 
   // IntegerLiterals are handled above and only EnumConstantDecls are expected
@@ -1081,6 +1105,10 @@
 // * Variable x is equal to the largest literal.
 // * Variable x is greater than largest literal.
 bool AlwaysTrue = true, AlwaysFalse = true;
+// Track value of both subexpressions.  If either side is always
+// true/false, another warning should have already been emitted.
+bool LHSAlwaysTrue = true, LHSAlwaysFalse = true;
+bool RHSAlwaysTrue = true, RHSAlwaysFalse = true;
 for (const llvm::APSInt  : Values) {
   TryResult Res1, Res2;
   Res1 = analyzeLogicOperatorCondition(BO1, Value, L1);
@@ -1096,10 +1124,16 @@
 AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
 AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
   }
+
+  LHSAlwaysTrue &= Res1.isTrue();
+  LHSAlwaysFalse &= Res1.isFalse();
+  RHSAlwaysTrue &= Res2.isTrue();
+  RHSAlwaysFalse &= Res2.isFalse();
 }
 
 if (AlwaysTrue || AlwaysFalse) {
-  if (BuildOpts.Observer)
+  if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
+  !RHSAlwaysFalse && BuildOpts.Observer)
 BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
   return TryResult(AlwaysTrue);
 }
Index: cfe/trunk/docs/ReleaseNotes.rst
===
--- cfe/trunk/docs/ReleaseNotes.rst
+++ cfe/trunk/docs/ReleaseNotes.rst
@@ -51,7 +51,8 @@
 Improvements to Clang's diagnostics
 ^^^
 
-- ...
+- -Wtautological-overlap-compare will warn on negative numbers and non-int
+  types.
 
 Non-comprehensive list of changes in this release
 -
Index: cfe/trunk/test/SemaCXX/warn-unreachable.cpp
===
--- cfe/trunk/test/SemaCXX/warn-unreachable.cpp
+++ 

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 219213.
rtrieu added a comment.

Add more test cases and split new warnings into a separate warning group, but 
still under -Wparentheses


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66043/new/

https://reviews.llvm.org/D66043

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/parentheses.c


Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -144,6 +144,28 @@
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '+'}} expected-note 2{{place parentheses}}
+
+  (void)(b ? 0xf0 : 0x10 | b ? 0x5 : 0x2); // expected-warning {{operator '?:' 
has lower precedence than '|'}} expected-note 2{{place parentheses}}
+
+  (void)((b ? 0xf0 : 0x10) | (b ? 0x5 : 0x2));  // no warning, has parentheses
+  (void)(b ? 0xf0 : (0x10 | b) ? 0x5 : 0x2);  // no warning, has parentheses
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '&'}} expected-note 2{{place parentheses}}
+
+  (void)((x | b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x | (b ? 1 : 2));  // no warning, has parentheses
+  (void)((x & b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x & (b ? 1 : 2));  // no warning, has parentheses
+
+  // Only warn on uses of the bitwise operators, and not the logical operators.
+  // The bitwise operators are more likely to be bugs while the logical
+  // operators are more likely to be used correctly.  Since there is no
+  // explicit logical-xor operator, the bitwise-xor is commonly used instead.
+  // For this warning, treat the bitwise-xor as if it were a logical operator.
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator
+  (void)(x && b ? 1 : 2);  // no warning, logical operator
 }
 
 // RUN: not %clang_cc1 -fsyntax-only -Wparentheses -Werror 
-fdiagnostics-show-option %s 2>&1 | FileCheck %s -check-prefix=CHECK-FLAG
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7485,7 +7485,12 @@
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return BinaryOperator::isAdditiveOp(Opc) ||
  BinaryOperator::isMultiplicativeOp(Opc) ||
- BinaryOperator::isShiftOp(Opc);
+ BinaryOperator::isShiftOp(Opc) || Opc == BO_And || Opc == BO_Or;
+  // This only checks for bitwise-or and bitwise-and, but not bitwise-xor and
+  // not any of the logical operators.  Bitwise-xor is commonly used as a
+  // logical-xor because there is no logical-xor operator.  The logical
+  // operators, including uses of xor, have a high false positive rate for
+  // precedence warnings.
 }
 
 /// IsArithmeticBinaryExpr - Returns true if E is an arithmetic binary
@@ -7575,7 +7580,11 @@
   // The condition is an arithmetic binary expression, with a right-
   // hand side that looks boolean, so warn.
 
-  Self.Diag(OpLoc, diag::warn_precedence_conditional)
+  unsigned DiagID = BinaryOperator::isBitwiseOp(CondOpCode)
+? diag::warn_precedence_bitwise_conditional
+: diag::warn_precedence_conditional;
+
+  Self.Diag(OpLoc, DiagID)
   << Condition->getSourceRange()
   << BinaryOperator::getOpcodeStr(CondOpcode);
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5618,6 +5618,9 @@
 def warn_precedence_conditional : Warning<
   "operator '?:' has lower precedence than '%0'; '%0' will be evaluated 
first">,
   InGroup;
+def warn_precedence_bitwise_conditional : Warning<
+  "operator '?:' has lower precedence than '%0'; '%0' will be evaluated 
first">,
+  InGroup;
 def note_precedence_conditional_first : Note<
   "place parentheses around the '?:' expression to evaluate it first">;
 
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -280,6 +280,7 @@
 def FlexibleArrayExtensions : DiagGroup<"flexible-array-extensions">;
 def FourByteMultiChar : DiagGroup<"four-char-constants">;
 def GlobalConstructors : DiagGroup<"global-constructors">;
+def BitwiseConditionalParentheses: 
DiagGroup<"bitwise-conditional-parentheses">;
 def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;
 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
 def LogicalNotParentheses: 

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked an inline comment as done.
rtrieu added a comment.

In D66044#1626008 , @NoQ wrote:

> Looks great, thanks! I'd appreciate direct CFG tests for the part that 
> changes the CFG (cf. `test/Analysis/cfg.cpp`), but i don't insist. Let's make 
> sure @jfb is happy and commit :)


I added a test case to show the new unreachable node in the CFG.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66044/new/

https://reviews.llvm.org/D66044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 215021.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66044/new/

https://reviews.llvm.org/D66044

Files:
  lib/Analysis/CFG.cpp
  lib/Analysis/ReachableCode.cpp
  test/Analysis/cfg.cpp
  test/Sema/warn-overlap.c
  test/Sema/warn-unreachable.c
  test/SemaCXX/warn-unreachable.cpp

Index: test/SemaCXX/warn-unreachable.cpp
===
--- test/SemaCXX/warn-unreachable.cpp
+++ test/SemaCXX/warn-unreachable.cpp
@@ -393,6 +393,9 @@
   else
 calledFun();// expected-warning {{will never be executed}}
 
+  if (y == -1 && y != -1)  // expected-note {{silence}}
+calledFun();// expected-warning {{will never be executed}}
+
   // TODO: Extend warning to the following code:
   if (x < -1)
 calledFun();
@@ -408,6 +411,4 @@
   else
 calledFun();
 
-  if (y == -1 && y != -1)
-calledFun();
 }
Index: test/Sema/warn-unreachable.c
===
--- test/Sema/warn-unreachable.c
+++ test/Sema/warn-unreachable.c
@@ -433,7 +433,7 @@
 }
 
 void unaryOpNoFixit() {
-  if (- 1)
+  if (~ 1)
 return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   unaryOpNoFixit(); // expected-warning {{code will never be executed}}
 }
Index: test/Sema/warn-overlap.c
===
--- test/Sema/warn-overlap.c
+++ test/Sema/warn-overlap.c
@@ -141,3 +141,20 @@
   return x < 1 || x != 0;
   // expected-warning@-1{{overlapping comparisons always evaluate to true}}
 }
+
+int integer_conversion(unsigned x, int y) {
+  return x > 4 || x < 10;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+  return y > 4u || y < 10u;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
+
+int negative_compare(int x) {
+  return x > -1 || x < 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
+
+int no_warning(unsigned x) {
+  return x >= 0 || x == 1;
+  // no warning since "x >= 0" is caught by a different tautological warning.
+}
Index: test/Analysis/cfg.cpp
===
--- test/Analysis/cfg.cpp
+++ test/Analysis/cfg.cpp
@@ -498,6 +498,26 @@
 }
 } // end namespace pr37688_deleted_union_destructor
 
+// CHECK-LABEL: int overlap_compare(int x)
+// CHECK: [B2]
+// CHECK-NEXT:   1: 1
+// CHECK-NEXT:  2: return [B2.1];
+// CHECK-NEXT:  Preds (1): B3(Unreachable)
+// CHECK-NEXT:  Succs (1): B0
+// CHECK: [B3]
+// CHECK-NEXT:   1: x
+// CHECK-NEXT:   2: [B3.1] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:   3: 5
+// CHECK-NEXT:   4: [B3.2] > [B3.3]
+// CHECK-NEXT:   T: if [B4.5] && [B3.4]
+// CHECK-NEXT:   Preds (1): B4
+// CHECK-NEXT:   Succs (2): B2(Unreachable) B1
+int overlap_compare(int x) {
+  if (x == -1 && x > 5)
+return 1;
+
+  return 2;
+}
 
 // CHECK-LABEL: template<> int *PR18472()
 // CHECK: [B2 (ENTRY)]
@@ -522,4 +542,3 @@
 void PR18472_helper() {
   PR18472();
 }
-
Index: lib/Analysis/ReachableCode.cpp
===
--- lib/Analysis/ReachableCode.cpp
+++ lib/Analysis/ReachableCode.cpp
@@ -247,7 +247,7 @@
 }
 case Stmt::UnaryOperatorClass: {
   const UnaryOperator *UO = cast(S);
-  if (UO->getOpcode() != UO_LNot)
+  if (UO->getOpcode() != UO_LNot && UO->getOpcode() != UO_Minus)
 return false;
   bool SilenceableCondValNotSet =
   SilenceableCondVal && SilenceableCondVal->getBegin().isInvalid();
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -70,11 +70,35 @@
   return D->getLocation();
 }
 
+/// Returns true on constant values based around a single IntegerLiteral.
+/// Allow for use of parentheses, integer casts, and negative signs.
+static bool IsIntegerLiteralConstantExpr(const Expr *E) {
+  // Allow parentheses
+  E = E->IgnoreParens();
+
+  // Allow conversions to different integer kind.
+  if (const auto *CE = dyn_cast(E)) {
+if (CE->getCastKind() != CK_IntegralCast)
+  return false;
+E = CE->getSubExpr();
+  }
+
+  // Allow negative numbers.
+  if (const auto *UO = dyn_cast(E)) {
+if (UO->getOpcode() != UO_Minus)
+  return false;
+E = UO->getSubExpr();
+  }
+
+  return isa(E);
+}
+
 /// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
-/// or EnumConstantDecl from the given Expr. If it fails, returns nullptr.
+/// constant expression or EnumConstantDecl from the given Expr. If it fails,
+/// returns nullptr.
 static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
   E = E->IgnoreParens();
-  if (isa(E))
+  if (IsIntegerLiteralConstantExpr(E))
 return E;
   if (auto *DR = dyn_cast(E->IgnoreParenImpCasts()))
 return isa(DR->getDecl()) ? DR : nullptr;
@@ -121,11 +145,11 @@
 static bool 

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked an inline comment as done.
rtrieu added inline comments.



Comment at: test/SemaCXX/self-comparison.cpp:87
+};
+} // namespace member_tests

jfb wrote:
> rtrieu wrote:
> > rtrieu wrote:
> > > jfb wrote:
> > > > The test only has `==`. Do other operators trigger?
> > > All the standard comparison operators will work here.  I'll add tests.
> > All operator tests added.
> `operator<=>`?
> 
> Is there a separate test for user-defined operators as well? What makes sense 
> in these cases? Technically a user-defined comparison could do anything... 
> but I think this warning makes sense for them as well.
operator<=> test was added to test/SemaCXX/compare-cxx2a.cpp because the 
operator requires header that this test doesn't include.

The warning currently doesn't check user-defined operators and I've include a 
test below to show that.  If a warning for user-defined operators was created, 
it would not belong in -Wtautological-compare because we would not know the 
actual result.  The best we could do is to say there was a self compare with a 
user-defined operator.  It would also need to hook into the AST at a different 
point.  Builtin compare operators are checked in BinaryOperator nodes.  
User-defined operators would need to be checked at CallExpr or 
CXXOperatorCallExpr instead.  This is different enough that it should be in a 
different patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66045/new/

https://reviews.llvm.org/D66045



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 215002.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66045/new/

https://reviews.llvm.org/D66045

Files:
  include/clang/AST/Expr.h
  lib/AST/Expr.cpp
  lib/Analysis/CFG.cpp
  lib/Sema/SemaExpr.cpp
  test/Analysis/array-struct-region.cpp
  test/Sema/warn-overlap.c
  test/SemaCXX/compare-cxx2a.cpp
  test/SemaCXX/self-comparison.cpp

Index: test/SemaCXX/self-comparison.cpp
===
--- test/SemaCXX/self-comparison.cpp
+++ test/SemaCXX/self-comparison.cpp
@@ -40,3 +40,84 @@
 Y::n == Y::n;
 }
 template bool g(); // should not produce any warnings
+
+namespace member_tests {
+struct B {
+  int field;
+  static int static_field;
+  int test(B b) {
+return field == field;  // expected-warning {{self-comparison always evaluates to true}}
+return static_field == static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return static_field == b.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return B::static_field == this->static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return this == this;  // expected-warning {{self-comparison always evaluates to true}}
+
+return field == b.field;
+return this->field == b.field;
+  }
+};
+
+enum {
+  I0,
+  I1,
+  I2,
+};
+
+struct S {
+  int field;
+  static int static_field;
+  int array[4];
+};
+
+struct T {
+  int field;
+  static int static_field;
+  int array[4];
+  S s;
+};
+
+int struct_test(S s1, S s2, S *s3, T t) {
+  return s1.field == s1.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s2.field == s2.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.static_field == s2.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return S::static_field == s1.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->field == s3->field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[0] == s1.array[0];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[I1] == s1.array[I1];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[s2.array[0]] == s1.array[s2.array[0]];  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->array[t.field] == s3->array[t.field];  // expected-warning {{self-comparison always evaluates to true}}
+
+  // Try all operators
+  return t.field == t.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.field <= t.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.field >= t.field;  // expected-warning {{self-comparison always evaluates to true}}
+
+  return t.field != t.field;  // expected-warning {{self-comparison always evaluates to false}}
+  return t.field < t.field;  // expected-warning {{self-comparison always evaluates to false}}
+  return t.field > t.field;  // expected-warning {{self-comparison always evaluates to false}}
+
+  // no warning
+  return s1.field == s2.field;
+  return s2.array == s1.array;
+  return s2.array[0] == s1.array[0];
+  return s1.array[I1] == s1.array[I2];
+
+  return s1.static_field == t.static_field;
+};
+
+struct U {
+  bool operator!=(const U&);
+};
+
+bool operator==(const U&, const U&);
+
+// May want to warn on this in the future.
+int user_defined(U u) {
+  return u == u;
+  return u != u;
+}
+
+} // namespace member_tests
Index: test/SemaCXX/compare-cxx2a.cpp
===
--- test/SemaCXX/compare-cxx2a.cpp
+++ test/SemaCXX/compare-cxx2a.cpp
@@ -8,12 +8,18 @@
 #define ASSERT_TYPE(...) static_assert(__is_same(__VA_ARGS__))
 #define ASSERT_EXPR_TYPE(Expr, Expect) static_assert(__is_same(decltype(Expr), Expect));
 
+struct S {
+  static int x[5];
+};
+
 void self_compare() {
   int a;
   int *b = nullptr;
+  S s;
 
   (void)(a <=> a); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
   (void)(b <=> b); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
+  (void)(s.x[a] <=> S::x[a]); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
 }
 
 void test0(long a, unsigned long b) {
Index: test/Sema/warn-overlap.c
===
--- test/Sema/warn-overlap.c
+++ test/Sema/warn-overlap.c
@@ -141,3 +141,17 @@
   return x < 1 || x != 0;
   // 

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked an inline comment as done.
rtrieu added inline comments.



Comment at: test/Sema/parentheses.c:148
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '&'}} expected-note 2{{place parentheses}}

MaskRay wrote:
> rtrieu wrote:
> > MaskRay wrote:
> > > I hope these `| ? :` `& ? :` warnings are disabled-by-default.
> > These new warnings reuse the existing parentheses warnings, which is 
> > diag::warn_precedence_conditional.  That is on by default, so this one as 
> > written is also on by default..
> I agree that 
> 
> `cond1 ? 0xf0 : 0x10 | cond2 ? 0x5 : 0x2;` is confusing and justifies a 
> warning. But **what is tested here is different**.
> 
> That is why I created D65192, because such warnings are very annoying as 
> enabled-by-default diagnostics.
> 
> I think this change will make it even harder to remove some annoying 
> -Wparentheses warnings..
I can add tests for the other case.  This one was used because it was shorter 
and easier to copy.

Would creating a parentheses subgroup (like -Wbitwise-conditional-parentheses), 
duping the warning into it, and marking it DefaultIgnore be a better 
alternative for you?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66043/new/

https://reviews.llvm.org/D66043



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked an inline comment as done.
rtrieu added inline comments.



Comment at: test/Sema/parentheses.c:156
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator

jfb wrote:
> rtrieu wrote:
> > jfb wrote:
> > > rtrieu wrote:
> > > > jfb wrote:
> > > > > I don't understand why `^` is different. What do you mean by "often 
> > > > > used as a logical xor`?
> > > > In C++, there's the bitwise operators |, &, and ^.  And there's the 
> > > > logical operators || and &&.  Since there's no ^^ for a logical-xor, 
> > > > many people will just use the bitwise-xor ^ instead.  Since this isn't 
> > > > warning on logical operators, it makes sense to exclude the bitwise-xor 
> > > > that is often used as logical-xor.
> > > So code is often buggy when it uses `|` and `&` as diagnosed by this 
> > > patch, but is rarely buggy when it uses `^`?
> > That's correct.  From my testing, &&, || and ^ all had low bug finding 
> > rates and didn't make sense to include into this warning while | and & had 
> > a high bug finding rate.
> OK thanks for clarifying. Could you explain this instead of "logical xor"? It 
> seems useful to know when reading your code that in your experience `^` 
> wasn't a source of bugs as much as the others.
I have added comments for this test and for the code where xor is excluded.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66043/new/

https://reviews.llvm.org/D66043



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 214753.
rtrieu added a comment.

Update comments to explain why bitwise-xor is treated as a logical operator.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66043/new/

https://reviews.llvm.org/D66043

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/parentheses.c


Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -144,6 +144,23 @@
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '+'}} expected-note 2{{place parentheses}}
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '&'}} expected-note 2{{place parentheses}}
+
+  (void)((x | b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x | (b ? 1 : 2));  // no warning, has parentheses
+  (void)((x & b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x & (b ? 1 : 2));  // no warning, has parentheses
+
+  // Only warn on uses of the bitwise operators, and not the logical operators.
+  // The bitwise operators are more likely to be bugs while the logical
+  // operators are more likely to be used correctly.  Since there is no
+  // explicit logical-xor operator, the bitwise-xor is commonly used instead.
+  // For this warning, treat the bitwise-xor as if it were a logical operator.
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator
+  (void)(x && b ? 1 : 2);  // no warning, logical operator
 }
 
 // RUN: not %clang_cc1 -fsyntax-only -Wparentheses -Werror 
-fdiagnostics-show-option %s 2>&1 | FileCheck %s -check-prefix=CHECK-FLAG
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7485,7 +7485,12 @@
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return BinaryOperator::isAdditiveOp(Opc) ||
  BinaryOperator::isMultiplicativeOp(Opc) ||
- BinaryOperator::isShiftOp(Opc);
+ BinaryOperator::isShiftOp(Opc) || Opc == BO_And || Opc == BO_Or;
+  // This only checks for bitwise-or and bitwise-and, but not bitwise-xor and
+  // not any of the logical operators.  Bitwise-xor is commonly used as a
+  // logical-xor because there is no logical-xor operator.  The logical
+  // operators, including uses of xor, have a high false positive rate for
+  // precedence warnings.
 }
 
 /// IsArithmeticBinaryExpr - Returns true if E is an arithmetic binary


Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -144,6 +144,23 @@
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '&'}} expected-note 2{{place parentheses}}
+
+  (void)((x | b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x | (b ? 1 : 2));  // no warning, has parentheses
+  (void)((x & b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x & (b ? 1 : 2));  // no warning, has parentheses
+
+  // Only warn on uses of the bitwise operators, and not the logical operators.
+  // The bitwise operators are more likely to be bugs while the logical
+  // operators are more likely to be used correctly.  Since there is no
+  // explicit logical-xor operator, the bitwise-xor is commonly used instead.
+  // For this warning, treat the bitwise-xor as if it were a logical operator.
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator
+  (void)(x && b ? 1 : 2);  // no warning, logical operator
 }
 
 // RUN: not %clang_cc1 -fsyntax-only -Wparentheses -Werror -fdiagnostics-show-option %s 2>&1 | FileCheck %s -check-prefix=CHECK-FLAG
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7485,7 +7485,12 @@
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return BinaryOperator::isAdditiveOp(Opc) ||
  BinaryOperator::isMultiplicativeOp(Opc) ||
- BinaryOperator::isShiftOp(Opc);
+ BinaryOperator::isShiftOp(Opc) || Opc == BO_And || Opc == BO_Or;
+  // This only checks for bitwise-or and bitwise-and, but not bitwise-xor and
+  // not any of the logical operators.  Bitwise-xor is commonly used as a
+  // logical-xor because there is no logical-xor operator.  The 

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked 3 inline comments as done.
rtrieu added inline comments.



Comment at: test/Analysis/array-struct-region.cpp:31
+  bool check() const { return this == this + 0; }
+  bool operator !() const { return this != this + 0; }
 

NoQ wrote:
> rtrieu wrote:
> > jfb wrote:
> > > Is this the only way? Or do casts also disable the diagnostic?
> > There's other ways to do this.  I picked "+ 0" here.  Explicit casts, like 
> > to void* or const S* would also disable it.  I think pragmas would also 
> > work.  I don't particularly care which way since this is an analyzer test.
> I'd rather disable the newly introduced warning on this test file, because 
> this is a path-sensitive static analysis test and this change may 
> accidentally ruin the original test.
Makes sense.  The warning is now disabled via -Wno flags on the RUN lines.



Comment at: test/SemaCXX/self-comparison.cpp:78
+  return S::static_field == s1.static_field;  // expected-warning 
{{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always 
evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning 
{{self-comparison always evaluates to true}}

rtrieu wrote:
> jfb wrote:
> > `s1.array[0] == s1.array[0]`?
> No, array accesses aren't checked yet.  But it's a good idea to look into it.
A little recursive checking and now we check array accesses.



Comment at: test/SemaCXX/self-comparison.cpp:87
+};
+} // namespace member_tests

rtrieu wrote:
> jfb wrote:
> > The test only has `==`. Do other operators trigger?
> All the standard comparison operators will work here.  I'll add tests.
All operator tests added.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66045/new/

https://reviews.llvm.org/D66045



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 214752.
rtrieu added a comment.

Check array accesses.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66045/new/

https://reviews.llvm.org/D66045

Files:
  include/clang/AST/Expr.h
  lib/AST/Expr.cpp
  lib/Analysis/CFG.cpp
  lib/Sema/SemaExpr.cpp
  test/Analysis/array-struct-region.cpp
  test/Sema/warn-overlap.c
  test/SemaCXX/self-comparison.cpp

Index: test/SemaCXX/self-comparison.cpp
===
--- test/SemaCXX/self-comparison.cpp
+++ test/SemaCXX/self-comparison.cpp
@@ -40,3 +40,71 @@
 Y::n == Y::n;
 }
 template bool g(); // should not produce any warnings
+
+namespace member_tests {
+struct B {
+  int field;
+  static int static_field;
+  int test(B b) {
+return field == field;  // expected-warning {{self-comparison always evaluates to true}}
+return static_field == static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return static_field == b.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return B::static_field == this->static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return this == this;  // expected-warning {{self-comparison always evaluates to true}}
+
+return field == b.field;
+return this->field == b.field;
+  }
+};
+
+enum {
+  I0,
+  I1,
+  I2,
+};
+
+struct S {
+  int field;
+  static int static_field;
+  int array[4];
+};
+
+struct T {
+  int field;
+  static int static_field;
+  int array[4];
+  S s;
+};
+
+int struct_test(S s1, S s2, S *s3, T t) {
+  return s1.field == s1.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s2.field == s2.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.static_field == s2.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return S::static_field == s1.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->field == s3->field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[0] == s1.array[0];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[I1] == s1.array[I1];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[s2.array[0]] == s1.array[s2.array[0]];  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->array[t.field] == s3->array[t.field];  // expected-warning {{self-comparison always evaluates to true}}
+
+  // Try all operators
+  return t.field == t.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.field <= t.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.field >= t.field;  // expected-warning {{self-comparison always evaluates to true}}
+
+  return t.field != t.field;  // expected-warning {{self-comparison always evaluates to false}}
+  return t.field < t.field;  // expected-warning {{self-comparison always evaluates to false}}
+  return t.field > t.field;  // expected-warning {{self-comparison always evaluates to false}}
+
+  // no warning
+  return s1.field == s2.field;
+  return s2.array == s1.array;
+  return s2.array[0] == s1.array[0];
+  return s1.array[I1] == s1.array[I2];
+
+  return s1.static_field == t.static_field;
+};
+} // namespace member_tests
Index: test/Sema/warn-overlap.c
===
--- test/Sema/warn-overlap.c
+++ test/Sema/warn-overlap.c
@@ -141,3 +141,17 @@
   return x < 1 || x != 0;
   // expected-warning@-1{{overlapping comparisons always evaluate to true}}
 }
+
+struct A {
+  int x;
+  int y;
+};
+
+int struct_test(struct A a) {
+  return a.x > 5 && a.y < 1;  // no warning, different variables
+
+  return a.x > 5 && a.x < 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+  return a.y == 1 || a.y != 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
Index: test/Analysis/array-struct-region.cpp
===
--- test/Analysis/array-struct-region.cpp
+++ test/Analysis/array-struct-region.cpp
@@ -1,20 +1,26 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN:-Wno-tautological-compare\
 // RUN:-x c %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN: 

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked 2 inline comments as done.
rtrieu added inline comments.



Comment at: lib/AST/Expr.cpp:3931-3932
+case DeclRefExprClass: {
+  // DeclRefExpr without an ImplicitCastExpr can happen for integral
+  // template parameters.
+  const auto *DRE1 = cast(E1);

NoQ wrote:
> Does this actually happen *after* instantiation? If not - do we even need to 
> emit warnings on templates that aren't instantiated? I guess we still need 
> this branch because `isSameComparisonOperand()` lives in the `Expr` class and 
> should always work regardless, but in this case i guess this deserves a 
> unittest(?)
This happens before instantiation.  The test cases are in 
(test/SemaTemplate/self-comparison.cpp).  The basic idea is to allow integral 
template parameters to be treated as variables.

```
template
void foo(int x) {
  bool b1 = A == A;
  bool b2 = x == x;
}
```

If we treat template parameter A the same as variable x, we can diagnose a few 
more tautological types, even without instantiation.



Comment at: lib/AST/Expr.cpp:3976
+if (const auto *D = dyn_cast(ME1->getMemberDecl()))
+  if (D->isStaticDataMember())
+return true;

NoQ wrote:
> Mmm, what are other cases when a `MemberExpr` doesn't have a `FieldDecl` as 
> its `getMemberDecl()`? Can this be turned into an `assert`?
I had to look this one up too.  From the docs for this method:

```
  /// Retrieve the member declaration to which this expression refers.
  ///
  /// The returned declaration will be a FieldDecl or (in C++) a VarDecl (for
  /// static data members), a CXXMethodDecl, or an EnumConstantDecl.
  ValueDecl *getMemberDecl() const { return MemberDecl; }
```

So, FieldDecl, VarDecl, CXXMethodDecl, and EnumConstantDecl are all valid 
return types.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66045/new/

https://reviews.llvm.org/D66045



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked 2 inline comments as done.
rtrieu added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161
+  "bitwise or with non-zero value always evaluates to true">,
+  InGroup, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<

jfb wrote:
> rtrieu wrote:
> > jfb wrote:
> > > Why default ignore?
> > This warning, like the tautological overlap warning, uses the CFG.  
> > CFG-based analysis are typically excluded from being default warnings due 
> > to the extra work of construction the CFG.
> Can you say so in the commit message?
Updated patch description.



Comment at: lib/Analysis/CFG.cpp:1118
+Expr::EvalResult Result;
+if (!Constant->EvaluateAsInt(Result, *Context))
+  return {};

NoQ wrote:
> It's kinda strange to me that we first confirm that it's a constant by doing 
> `tryTransformToIntOrEnumConstant`, but then fire up the heavy machinery of 
> `EvaluateAsInt` anyway. Did you consider using only `EvaluateAsInt()` to 
> begin with? I guess you're trying to make sure that "the user's intent is 
> clear" as other similar warnings do, right? Could you comment on that?
A new function has been created to check the zero-ness of the Expr.  Since we 
know it the Expr comes from tryTransformToIntOrEnumConstant, this function 
doesn't have to handle the full Expr sub-classes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66046/new/

https://reviews.llvm.org/D66046



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 214733.
rtrieu added a comment.

Create new function isIntOrEnumConstantZero to use instead of EvaluateAsInt


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66046/new/

https://reviews.llvm.org/D66046

Files:
  include/clang/Analysis/CFG.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/CFG.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  test/Sema/warn-bitwise-compare.c
  test/SemaCXX/warn-bitwise-compare.cpp

Index: test/SemaCXX/warn-bitwise-compare.cpp
===
--- test/SemaCXX/warn-bitwise-compare.cpp
+++ test/SemaCXX/warn-bitwise-compare.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+
+void test(int x) {
+  bool b1 = (8 & x) == 3;
+  // expected-warning@-1 {{bitwise comparison always evaluates to false}}
+  bool b2 = x | 5;
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b3 = (x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b4 = !!(x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+}
Index: test/Sema/warn-bitwise-compare.c
===
--- test/Sema/warn-bitwise-compare.c
+++ test/Sema/warn-bitwise-compare.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 
 #define mydefine 2
 
@@ -13,6 +13,9 @@
   if ((x & 0x15) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
   if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
 
+  if (!!((8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  int y = ((8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((x & 8) == 8) {}
   if ((x & 8) != 8) {}
   if ((x | 4) == 4) {}
@@ -26,3 +29,9 @@
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
 }
+
+void g(int x) {
+  if (x | 5) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (5 | x) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (!((x | 5))) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+}
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -159,6 +159,20 @@
 S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_always)
 << DiagRange << isAlwaysTrue;
   }
+
+  void compareBitwiseOr(const BinaryOperator *B) override {
+if (HasMacroID(B))
+  return;
+
+SourceRange DiagRange = B->getSourceRange();
+S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_or) << DiagRange;
+  }
+
+  static bool hasActiveDiagnostics(DiagnosticsEngine ,
+   SourceLocation Loc) {
+return !Diags.isIgnored(diag::warn_tautological_overlap_comparison, Loc) ||
+   !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc);
+  }
 };
 } // anonymous namespace
 
@@ -2076,10 +2090,9 @@
   .setAlwaysAdd(Stmt::AttributedStmtClass);
   }
 
-  // Install the logical handler for -Wtautological-overlap-compare
+  // Install the logical handler.
   llvm::Optional LEH;
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-   D->getBeginLoc())) {
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
 LEH.emplace(S);
 AC.getCFGBuildOptions().Observer = &*LEH;
   }
@@ -2228,9 +2241,8 @@
 checkThrowInNonThrowingFunc(S, FD, AC);
 
   // If none of the previous checks caused a CFG build, trigger one here
-  // for -Wtautological-overlap-compare
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-   D->getBeginLoc())) {
+  // for the logical error handler.
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
 AC.getCFG();
   }
 
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -81,6 +81,20 @@
   return nullptr;
 }
 
+/// Takes an expression returned from tryTransformToIntOrEnumConstant and
+/// evaluates its non-zeroness.
+static bool isIntOrEnumConstantZero(const Expr *E) {
+  E = E->IgnoreParens();
+  if (const auto *Integer = dyn_cast(E))
+return Integer->getValue() == 0;
+
+  // These are non-null because they have been checked in
+  // tryTransformToIntOrEnumConstant
+  const auto *DR = cast(E->IgnoreParenImpCasts());
+  const auto *EC = cast(DR->getDecl());
+  return EC->getInitVal() == 0;
+}
+
 /// Tries to interpret a 

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked an inline comment as done.
rtrieu added inline comments.



Comment at: test/Sema/parentheses.c:156
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator

jfb wrote:
> rtrieu wrote:
> > jfb wrote:
> > > I don't understand why `^` is different. What do you mean by "often used 
> > > as a logical xor`?
> > In C++, there's the bitwise operators |, &, and ^.  And there's the logical 
> > operators || and &&.  Since there's no ^^ for a logical-xor, many people 
> > will just use the bitwise-xor ^ instead.  Since this isn't warning on 
> > logical operators, it makes sense to exclude the bitwise-xor that is often 
> > used as logical-xor.
> So code is often buggy when it uses `|` and `&` as diagnosed by this patch, 
> but is rarely buggy when it uses `^`?
That's correct.  From my testing, &&, || and ^ all had low bug finding rates 
and didn't make sense to include into this warning while | and & had a high bug 
finding rate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66043/new/

https://reviews.llvm.org/D66043



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked 3 inline comments as done.
rtrieu added a comment.

In D66045#1624389 , @jfb wrote:

> Does this work for unions as well?


This will work for union accesses via the same member name, but not different 
member names.

  union U { int x; int y; } u;
  bool b1 = u.x == u.x;  // warn always true
  bool b2 = u.x == u.y;  // no warning

It's easy to do when the types are the same, but more difficult when the types 
are different, so keeping with the same name member access is the safe way for 
now.




Comment at: test/Analysis/array-struct-region.cpp:31
+  bool check() const { return this == this + 0; }
+  bool operator !() const { return this != this + 0; }
 

jfb wrote:
> Is this the only way? Or do casts also disable the diagnostic?
There's other ways to do this.  I picked "+ 0" here.  Explicit casts, like to 
void* or const S* would also disable it.  I think pragmas would also work.  I 
don't particularly care which way since this is an analyzer test.



Comment at: test/SemaCXX/self-comparison.cpp:78
+  return S::static_field == s1.static_field;  // expected-warning 
{{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always 
evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning 
{{self-comparison always evaluates to true}}

jfb wrote:
> `s1.array[0] == s1.array[0]`?
No, array accesses aren't checked yet.  But it's a good idea to look into it.



Comment at: test/SemaCXX/self-comparison.cpp:87
+};
+} // namespace member_tests

jfb wrote:
> The test only has `==`. Do other operators trigger?
All the standard comparison operators will work here.  I'll add tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66045/new/

https://reviews.llvm.org/D66045



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked an inline comment as done.
rtrieu added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161
+  "bitwise or with non-zero value always evaluates to true">,
+  InGroup, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<

jfb wrote:
> Why default ignore?
This warning, like the tautological overlap warning, uses the CFG.  CFG-based 
analysis are typically excluded from being default warnings due to the extra 
work of construction the CFG.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66046/new/

https://reviews.llvm.org/D66046



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked an inline comment as done.
rtrieu added inline comments.



Comment at: test/Sema/warn-unreachable.c:437
+  if (~ 1)
 return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   unaryOpNoFixit(); // expected-warning {{code will never be executed}}

jfb wrote:
> Why this change?
```
​  if (y == -1 && y != -1)  // condition from warn-unreachable.cpp
  if (- 1)  // condition here
```

The test case below in warn-unreachable.cpp all have silence notes on them.  
For consistency, I wanted the new case involving (y == -1 && y != -1) to also 
have a silence case.  The change to the reachability analysis to do that would 
also generate a silence note for (- 1).  Since (- 1) now generates a silence, I 
picked a different unary operator that would not.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66044/new/

https://reviews.llvm.org/D66044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked 2 inline comments as done.
rtrieu added inline comments.



Comment at: test/Sema/parentheses.c:148
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '&'}} expected-note 2{{place parentheses}}

MaskRay wrote:
> I hope these `| ? :` `& ? :` warnings are disabled-by-default.
These new warnings reuse the existing parentheses warnings, which is 
diag::warn_precedence_conditional.  That is on by default, so this one as 
written is also on by default..



Comment at: test/Sema/parentheses.c:156
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator

jfb wrote:
> I don't understand why `^` is different. What do you mean by "often used as a 
> logical xor`?
In C++, there's the bitwise operators |, &, and ^.  And there's the logical 
operators || and &&.  Since there's no ^^ for a logical-xor, many people will 
just use the bitwise-xor ^ instead.  Since this isn't warning on logical 
operators, it makes sense to exclude the bitwise-xor that is often used as 
logical-xor.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66043/new/

https://reviews.llvm.org/D66043



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63789/new/

https://reviews.llvm.org/D63789



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/AST/ODRHash.cpp:73
 AddBoolean(S.isUnarySelector());
 unsigned NumArgs = S.getNumArgs();
 for (unsigned i = 0; i < NumArgs; ++i) {

vsapsai wrote:
> rtrieu wrote:
> > There's actually a second bug here as well.  When processing an arbitrary 
> > number of elements, the number of elements needs to placed before the list. 
> >  This line should be added before the for-loop:
> > 
> > ```
> > ID.AddInteger(NumArgs);
> > 
> > ```
> > This change isn't directly related to your original patch, so feel free to 
> > skip it.
> Thanks for the review, Richard. What would be the way to test the suggested 
> changes? I was mostly thinking about making sure we treat as different
> * `-foo:` and `-foo::`
> * `-foo:bar::` and `-foo::bar:`
> 
> Does it sound reasonable? Maybe you have some test examples for C++ that I 
> can adopt for Objective-C.
Yes, checking things close to each other sounds reasonable.   I did pretty much 
the same thing testing in C++, find things (usually types) that are close to 
others and making sure the ODRHash can tell them apart.  The ObjC testing is a 
little sparse since I was focusing on the C++ part.  Thanks for helping fill it 
out.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63789/new/

https://reviews.llvm.org/D63789



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/AST/ODRHash.cpp:73
 AddBoolean(S.isUnarySelector());
 unsigned NumArgs = S.getNumArgs();
 for (unsigned i = 0; i < NumArgs; ++i) {

There's actually a second bug here as well.  When processing an arbitrary 
number of elements, the number of elements needs to placed before the list.  
This line should be added before the for-loop:

```
ID.AddInteger(NumArgs);

```
This change isn't directly related to your original patch, so feel free to skip 
it.



Comment at: clang/lib/AST/ODRHash.cpp:75
 for (unsigned i = 0; i < NumArgs; ++i) {
-  AddIdentifierInfo(S.getIdentifierInfoForSlot(i));
+  ID.AddString(S.getNameForSlot(i));
 }

For possibly null pointers, ODRHash uses a boolean before processing the 
pointer.  The way to fix this is:

```
const IdentifierInfo *II = S.getIdentifierInfoForSlot(i);
AddBoolean(II)
if (II) {
  AddIdentifierInfo(II);
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63789/new/

https://reviews.llvm.org/D63789



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58122: Restore Check for Unreachable Exit Block in -Winfinite-recursion

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58122/new/

https://reviews.llvm.org/D58122



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52218: Warn on self-initializations

[Richard Trieu via Phabricator via cfe-commits]

rtrieu created this revision.

Improve some diagnostics around self-initialization.  The uninitialized checker 
does not warn on self-initialization, but does warn later if the variable is 
used.  GCC also has a -Winit-self which will enable a -Wuninitialized warning 
for self-initialized.  -Winit-self is active in -Wall

There's also the matter of the variable being const.  GCC -Wuninitialized will 
warn on a const self-initialized with or without -Winit-self.

This patch will make it so that const self-initialization is always warned on 
under -Wuninitialized.  Since Clang does not use warning flags to control other 
warnings, -Winit-self will be a new warning group for non-const 
self-initialized.  -Winit-self will also be in -Wall via -Wmost.

  // GCC - warn with -Wuninitialized and -Winit-self
  // Clang - no warning
  // Clang with patch - warn with -Winit-self
  void f1() {
int x = x;
  }
  
  // GCC - warn with -Wuninitialized
  // Clang - no warning
  // Clang with patch - warn with -Wuninitialized
  void f2() {
const int x = x;
  }




https://reviews.llvm.org/D52218

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/SemaDecl.cpp
  test/SemaCXX/uninitialized.cpp

Index: test/SemaCXX/uninitialized.cpp
===
--- test/SemaCXX/uninitialized.cpp
+++ test/SemaCXX/uninitialized.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wuninitialized -Wno-unused-value -Wno-unused-lambda-capture -std=c++1z -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wuninitialized -Wno-unused -std=c++1z -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wno-unused -Wno-init-self -std=c++1z -verify %s
 
 // definitions for std::move
 namespace std {
@@ -1432,7 +1433,7 @@
 void array_capture(bool b) {
   const char fname[] = "array_capture";
   if (b) {
-int unused; // expected-warning {{unused variable}}
+int unused;
   } else {
 [fname]{};
   }
@@ -1447,3 +1448,15 @@
 
   switch (int n; (n == k || k > 5)) {} // expected-warning {{uninitialized}} expected-note {{initialize}} expected-warning {{boolean}}
 }
+
+namespace const_self_init {
+const int a = a;  // no-warning for global
+
+void const_init() {
+  const int a = a; // expected-warning {{variable 'a' is uninitialized when used within its own initialization}}
+
+  const int b = b; // expected-warning {{variable 'b' is uninitialized when used within its own initialization}}
+  int c = b;
+}
+
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -1,6 +1,19 @@
 Init = Result.getAs();
   }
 
+  if (!VDecl->getType().isConstQualified()) {
+if (auto *SubExpr = dyn_cast(Init->IgnoreParens())) {
+  if (SubExpr->getCastKind() == CK_LValueToRValue) {
+if (auto *DRE = dyn_cast(SubExpr->getSubExpr())) {
+  if (DRE->getDecl() == VDecl) {
+Diag(VDecl->getLocation(), diag::warn_uninit_self_init)
+<< VDecl << VDecl->getSourceRange() << DRE->getSourceRange();
+  }
+}
+  }
+}
+  }
+
   // Check for self-references within variable initializers.
   // Variables declared within a function/method body (except for references)
   // are handled by a dataflow analysis.
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -977,7 +977,8 @@
 // uninitialized. Proven code paths which access 'x' in
 // an uninitialized state after this will still warn.
 if (const Expr *Initializer = VD->getInit()) {
-  if (!alwaysReportSelfInit && DRE == Initializer->IgnoreParenImpCasts())
+  if (!VD->getType().isConstQualified() && !alwaysReportSelfInit &&
+  DRE == Initializer->IgnoreParenImpCasts())
 return false;
 
   ContainsReference CR(S.Context, DRE);
@@ -1518,10 +1519,17 @@
   UsesVec *vec = V.getPointer();
   bool hasSelfInit = V.getInt();
 
-  // Specially handle the case where we have uses of an uninitialized
-  // variable, but the root cause is an idiomatic self-init.  We want
-  // to report the diagnostic at the self-init since that is the root cause.
-  if (!vec->empty() && hasSelfInit && hasAlwaysUninitializedUse(vec))
+  if (hasSelfInit && vd->getType().isConstQualified()) {
+// Handle "const int a = a;"
+DiagnoseUninitializedUse(S, vd,
+ UninitUse(vd->getInit()->IgnoreParenCasts(),
+   /* isAlwaysUninit */ true),
+ /* alwaysReportSelfInit */ true);
+  } else if (!vec->empty() && hasSelfInit && hasAlwaysUninitializedUse(vec))
+// Specially handle the case where we have uses of an uninitialized
+ 

[PATCH] D48524: [ODRHash] Rip out the registration of Type* in TypeMap

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

LGTM

Thank you for reducing this test case for ODR hashing.


https://reviews.llvm.org/D48524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48524: [ODRHash] Do not put elaborated types in the TypeMap

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Hey Vassil,

I have been giving this a little more thought and I have a solution I'd like 
you to try out.  If the type pointers aren't unique enough, then the indexing 
scheme will not work for hashing.  Can you try this idea in your build 
environment and see if works for you?  That is,  use this version AddType:

  void ODRHash::AddType(const Type *T) {
assert(T && "Expecting non-null pointer.");
  
ODRTypeVisitor(ID, *this).Visit(T);
  }

If the recursion issue that originally made the indexing scheme necessary is no 
longer present, then this would be path forward for ODR Hashing.


Repository:
  rC Clang

https://reviews.llvm.org/D48524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48524: [ODRHash] Do not put elaborated types in the TypeMap

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Running this, I see that it causes a regression in one of the other ODR Hash 
tests.  Specifically, the diagnostics on line 1150 & 1151 of 
test/Modules/odr_hash.cpp are not seen.  Was that an expected side-effect of 
this patch?


Repository:
  rC Clang

https://reviews.llvm.org/D48524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47340: [ClangDiagnostics] Silence warning about fallthrough after PrintFatalError

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

lgtm


https://reviews.llvm.org/D47340



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47340: [ClangDiagnostics] Silence warning about fallthrough after PrintFatalError

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Instead of adding a break, you should add a noreturn attribute to the 
PrintFatalError function.  LLVM has the macro LLVM_ATTRIBUTE_NORETURN to do 
this.  You can confirm that PrintFatalError is a noreturn function by seeing 
that it unconditionally calls llvm::PrintFatalError which itself is attributed 
with LLVM_ATTRIBUTE_NORETURN.


Repository:
  rC Clang

https://reviews.llvm.org/D47340



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

r328404 has been committed to support references and pointers.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43737: Improve -Winfinite-recursion

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

Looks good.  Ready to commit.


https://reviews.llvm.org/D43737



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.

[Richard Trieu via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL327598: Refactoring code around move/copy initialization.  
NFC. (authored by rtrieu, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43898?vs=137920=138488#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43898

Files:
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaExprCXX.cpp
  cfe/trunk/lib/Sema/SemaStmt.cpp
  cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -3795,10 +3795,18 @@
   RecordDecl *CreateCapturedStmtRecordDecl(CapturedDecl *,
SourceLocation Loc,
unsigned NumParams);
+
+  enum CopyElisionSemanticsKind {
+CES_Strict = 0,
+CES_AllowParameters = 1,
+CES_AllowDifferentTypes = 2,
+CES_Default = (CES_AllowParameters | CES_AllowDifferentTypes),
+  };
+
   VarDecl *getCopyElisionCandidate(QualType ReturnType, Expr *E,
-   bool AllowParamOrMoveConstructible);
+   CopyElisionSemanticsKind CESK);
   bool isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-  bool AllowParamOrMoveConstructible);
+  CopyElisionSemanticsKind CESK);
 
   StmtResult ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
  Scope *CurScope);
Index: cfe/trunk/lib/Sema/SemaStmt.cpp
===
--- cfe/trunk/lib/Sema/SemaStmt.cpp
+++ cfe/trunk/lib/Sema/SemaStmt.cpp
@@ -2862,16 +2862,16 @@
 /// \param E The expression being returned from the function or block, or
 /// being thrown.
 ///
-/// \param AllowParamOrMoveConstructible Whether we allow function parameters or
+/// \param CESK Whether we allow function parameters or
 /// id-expressions that could be moved out of the function to be considered NRVO
 /// candidates. C++ prohibits these for NRVO itself, but we re-use this logic to
 /// determine whether we should try to move as part of a return or throw (which
 /// does allow function parameters).
 ///
 /// \returns The NRVO candidate variable, if the return statement may use the
 /// NRVO, or NULL if there is no such candidate.
 VarDecl *Sema::getCopyElisionCandidate(QualType ReturnType, Expr *E,
-   bool AllowParamOrMoveConstructible) {
+   CopyElisionSemanticsKind CESK) {
   if (!getLangOpts().CPlusPlus)
 return nullptr;
 
@@ -2884,29 +2884,29 @@
   if (!VD)
 return nullptr;
 
-  if (isCopyElisionCandidate(ReturnType, VD, AllowParamOrMoveConstructible))
+  if (isCopyElisionCandidate(ReturnType, VD, CESK))
 return VD;
   return nullptr;
 }
 
 bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-  bool AllowParamOrMoveConstructible) {
+  CopyElisionSemanticsKind CESK) {
   QualType VDType = VD->getType();
   // - in a return statement in a function with ...
   // ... a class return type ...
   if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
 if (!ReturnType->isRecordType())
   return false;
 // ... the same cv-unqualified type as the function return type ...
 // When considering moving this expression out, allow dissimilar types.
-if (!AllowParamOrMoveConstructible && !VDType->isDependentType() &&
+if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
 !Context.hasSameUnqualifiedType(ReturnType, VDType))
   return false;
   }
 
   // ...object (other than a function or catch-clause parameter)...
   if (VD->getKind() != Decl::Var &&
-  !(AllowParamOrMoveConstructible && VD->getKind() == Decl::ParmVar))
+  !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar))
 return false;
   if (VD->isExceptionVariable()) return false;
 
@@ -2918,7 +2918,7 @@
   // variable will no longer be used.
   if (VD->hasAttr()) return false;
 
-  if (AllowParamOrMoveConstructible)
+  if (CESK & CES_AllowDifferentTypes)
 return true;
 
   // ...non-volatile...
@@ -2933,6 +2933,71 @@
   return true;
 }
 
+/// \brief Try to perform the initialization of a potentially-movable value,
+/// which is the operand to a return or throw statement.
+///
+/// This routine implements C++14 [class.copy]p32, which attempts to treat
+/// returned lvalues as rvalues in certain cases (to prefer move construction),
+/// then falls back to treating them as lvalues if that failed.
+///
+/// \param Res We will fill this in if move-initialization was 

[PATCH] D43737: Improve -Winfinite-recursion

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Two more changes, then everything is good to commit.




Comment at: lib/Sema/AnalysisBasedWarnings.cpp:218-220
+// Found a path to the exit node without a recursive call.
+if (ExitID == Block->getBlockID())
+  return false;

Move this to checking the ID of the successor block instead of the current 
block.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:227-233
+// If the successor block contains a recursive call, end analysis 
there.
+if (!hasRecursiveCallInPath(FD, *SuccBlock)) {
+  WorkList.push_back(SuccBlock);
+  continue;
 }
+
+foundRecursion = true;

This would make more sense if you flip the conditional:


```
if (hasRecursiveCallInPath(FD, *SuccBlock)) {
  foundRecursion = true;
  continue;
}

WorkList.push_back(SuccBlock);
```


https://reviews.llvm.org/D43737



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Everything looks good and ready for commit.


Repository:
  rC Clang

https://reviews.llvm.org/D43898



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43737: Improve -Winfinite-recursion

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

>   I believe you were around this code last, so can you remember why it was 
> there?

Yes, that's an early exit to speed up the check.  You can remove that check and 
add a test case for it.

This was a little confusing for me, so let's refactor it a little.  These 
changes will make it so that any CFGBlock in the WorkList has already been 
checked, so it can go straight to checking the successors.




Comment at: lib/Sema/AnalysisBasedWarnings.cpp:203-204
 
-// All blocks are in one of three states.  States are ordered so that blocks
-// can only move to higher states.
-enum RecursiveState {
-  FoundNoPath,
-  FoundPath,
-  FoundPathWithNoRecursiveCall
-};
-
 // Returns true if there exists a path to the exit block and every path
 // to the exit block passes through a call to FD.
 static bool checkForRecursiveFunctionCall(const FunctionDecl *FD, CFG *cfg) {

Update comment to reflect your changes.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:224-225
 
-  // Mark all nodes as FoundNoPath, then set the status of the entry block.
-  SmallVector States(cfg->getNumBlockIDs(), FoundNoPath);
-  States[cfg->getEntry().getBlockID()] = FoundPathWithNoRecursiveCall;
-
-  // Make the processing stack and seed it with the entry block.
-  SmallVector Stack;
-  Stack.push_back(>getEntry());
-
-  while (!Stack.empty()) {
-CFGBlock *CurBlock = Stack.back();
-Stack.pop_back();
+  // Seed the work list with the entry block.
+  foundRecursion |= analyzeSuccessor(>getEntry());
 

The entry CFGBlock is special.  It will never trigger on any of the conditions 
we are checking for.  Just push it into WorkList and Visited.  It has no 
statements, so it can't have a recursive call.

http://clang.llvm.org/docs/InternalsManual.html#entry-and-exit-blocks

Technically, it has no incoming edges, so you don't even need to insert it into 
Visited.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:232-233
+// Found a path to the exit node without a recursive call.
+if (ExitID == ID)
+  return false;
 

ID is not longer needed as an index so it is only needed here, so it should be 
inlined.

Also, move this check in the loop over the successors, so there are no checks 
on the current CFG block.  This way, all checks happen at the same place.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:237
+  if (*I)
+foundRecursion |= analyzeSuccessor(*I);
   }

Since the lambda isn't needed for seeding the WorkList, this is the only use 
and it should be inlined here.

Also, use:
```
   if (cond)
 foundRecursion = true;
```
instead of:

```
  foundRecursion |= true;
```


https://reviews.llvm.org/D43737



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40731: Integrate CHash into CLang

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: include/clang/AST/CHashVisitor.h:72-79
+  template
+  void addData(const llvm::iterator_range ) {
+addData(std::distance(x.begin(), x.end()));
+  }
+  template
+  void addData(const llvm::ArrayRef ) {
+addData(x.size());

Should these also be storing the hashes of the elements in addition to storing 
the size?



Comment at: include/clang/AST/CHashVisitor.h:211
+
+  bool VisitTypeDecl(TypeDecl *D) {
+// If we would hash the resulting type for a typedef, we

I thought the idea was to have all the code in the *Collectors.td but here you 
have a bunch of Visit* functions for AST nodes.  This seems counter to your 
point of having them generated.



Comment at: include/clang/AST/CHashVisitor.h:212-213
+  bool VisitTypeDecl(TypeDecl *D) {
+// If we would hash the resulting type for a typedef, we
+// would get into an endless recursion.
+if (!isa(D) && !isa(D) && !isa(D)) {

I don't see this example in the test.



Comment at: include/clang/AST/CHashVisitor.h:226
+}
+if (isa(ValDecl)) {
+  /* We emulate TraverseDecl here for VarDecl, because we

Can't you query the hash for the Decl instead of doing work here?



Comment at: include/clang/AST/CHashVisitor.h:253
+  bool VisitValueDecl(ValueDecl *D) {
+/* Field Declarations can induce recursions */
+if (isa(D)) {

Is this possible recursion included in your test?



Comment at: include/clang/AST/CHashVisitor.h:255
+if (isa(D)) {
+  addData(std::string(D->getType().getAsString()));
+} else {

Shouldn't this be handled in the VisitFieldDecl function?



Comment at: include/clang/AST/DeclDataCollectors.td:5-7
+  // Every Declaration gets a tag field in the hash stream. It is
+  // hashed to add additional randomness to the hash
+  addData(llvm::hash_value(S->getKind()));

Why is this randomness needed?



Comment at: include/clang/AST/DeclDataCollectors.td:9
+
+  // CrossRef
+  addData(S->hasAttrs());

What do you mean by "CrossRef"?



Comment at: include/clang/AST/DeclDataCollectors.td:60-61
+  code Code = [{
+   /* Not every enum has a init expression. Therefore, 
+  we extract the actual enum value from it. */
+   addData(S->getInitVal().getExtValue());

Does that mean you consider:

```
enum E { E1, E2, E3 };
```
to be equivalent to:

```
enum E { E1 = 0, E2 = 1, E3 = 2 };
```
?


Repository:
  rC Clang

https://reviews.llvm.org/D40731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: lib/Sema/SemaStmt.cpp:2953
+  ExprResult )
+{
+  ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),

Opening brace should follow the closing paren on previous line.



Comment at: lib/Sema/SemaStmt.cpp:2963
+  InitializationSequence Seq(S, Entity, Kind, InitExpr);
+  if (Seq) {
+for (const InitializationSequence::Step  : Seq.steps()) {

Use early exit here:


```
if (!Seq)
  return;

// rest of function
```



Comment at: lib/Sema/SemaStmt.cpp:2965-2966
+for (const InitializationSequence::Step  : Seq.steps()) {
+  if (!(Step.Kind == InitializationSequence::SK_ConstructorInitialization 
||
+Step.Kind == InitializationSequence::SK_UserConversion))
+continue;

Since you're simplifying the condition here, bring the not operator inside the 
parentheses.


```
if (Step.Kind != ...  && Step.Kind != ...)
```



Comment at: lib/Sema/SemaStmt.cpp:2970
+  FunctionDecl *FD = Step.Function.Function;
+  if (isa(FD)) {
+// C++14 [class.copy]p32:

Use early exit here:


```
if (!isa(FD)
  continue;

// old if-block code
```



Comment at: lib/Sema/SemaStmt.cpp:2999-3000
-// expression node to persist.
-Value = ImplicitCastExpr::Create(Context, Value->getType(), CK_NoOp,
- Value, nullptr, VK_XValue);
 

At this point, the variable Value is updated.  Value is scoped to this 
function, and used again after this code.  In your change, there is a new Value 
variable in the static function.  Only that variable is updated and not this 
one, making this a change in behavior.


Repository:
  rC Clang

https://reviews.llvm.org/D43898



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: lib/AST/ODRHash.cpp:581
+  void VisitType(const Type *T) {
+ID.AddInteger(T->getTypeClass());
+  }

v.g.vassilev wrote:
> rtrieu wrote:
> > rsmith wrote:
> > > This looks redundant, the above `Visit(const Type*)` function seems to 
> > > already do this.
> > That's correct, VisitType is intended to be empty.  The TypeClass enum 
> > value is added in Visit so that it is the first value added to the data 
> > stream.
> Ok, then I am a little confused. If `VisitType` is supposed to be nop why we 
> call it in all VisitXXX functions.
Each Type calls its parent Type, all the way up to Type.  It's just a manual 
traversal of the Type hierarchy.  Right now, there's nothing in VisitType, but 
there might be in the future.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43737: Improve -Winfinite-recursion

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:255-257
   // If the exit block is unreachable, skip processing the function.
   if (cfg->getExit().pred_empty())
 return;

This is likely what is causing my previous code example to fail.


https://reviews.llvm.org/D43737



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43737: Improve -Winfinite-recursion

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Can you explain the new algorithm for checking recursive calls?

From the test, this code looks like what you are testing for, but it doesn't 
trigger the warning.

  void f() {
while(true) {
  f();
}
  } 


https://reviews.llvm.org/D43737



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.



> I have one very minor nit that I don't know how to fix:
> 
>   warn-return-std-move.cpp:220:12: warning: local variable 'd' will be copied
> despite being returned by name [-Wreturn-std-move]
>   return (d);  // e17
>  ^
>   warn-return-std-move.cpp:220:12: note: call 'std::move' explicitly to avoid 
> copying
>   return (d);  // e17
>  ^~~
>  std::move(d)
> 
> 
> The warning places a caret directly under the `(`, when I wish it would place 
> `^~~` under the entire expression, the way the fixit does.

You can add extra tildes to any diagnostic by passing a SourceRange to Diag() 
calls the same way you pass FixitHints.




Comment at: lib/Sema/SemaStmt.cpp:2937
 
+static void AttemptMoveInitialization(Sema& S,
+  const InitializedEntity ,

I have a few concerns about this function.  The code isn't a straight move from 
the old location to this one, so it is hard to follow along the changes, 
especially the change to the complex if conditional.  The function should be 
commented, especially to behavior difference for setting IsFake.

It looks like when IsFake is set, then the result is discarded and not used, 
but it is still possible on success for AsRvalue to be copied to the heap.  
This is wasteful when it is never used again.

Another issue is the Value in the original code is used again towards the end 
of the function on line #3013.  In the old code, Value can be updated while in 
the new code, it does.

It may be better to split this change in two, the first adding this function 
and the CopyElisionSemanticsKind enum and the second adding the diagnostic 
itself.


Repository:
  rC Clang

https://reviews.llvm.org/D43322



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: lib/AST/ODRHash.cpp:581
+  void VisitType(const Type *T) {
+ID.AddInteger(T->getTypeClass());
+  }

rsmith wrote:
> This looks redundant, the above `Visit(const Type*)` function seems to 
> already do this.
That's correct, VisitType is intended to be empty.  The TypeClass enum value is 
added in Visit so that it is the first value added to the data stream.



Comment at: lib/AST/ODRHash.cpp:590
+  void VisitReferenceType(const ReferenceType *T) {
+AddQualType(T->getPointeeType());
+VisitType(T);

Use T->getPointeeTypeAsWritten() here.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34810: [Sema] -Wcomma should not warn for expressions that return void

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Reid is correct, the whitelisted expressions was greatly reduced during code 
review so only casts to void would disable the warning.  While the last review 
did not have the description updated to reflect this, the committed code does 
have an accurate description.  What is the reason to exclude void expressions 
now?  For the function case, it is more consistent to warn on all function 
calls since we can't determine if a function returns void just by looking at 
the call site.


Repository:
  rL LLVM

https://reviews.llvm.org/D34810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21675: New ODR checker for modules

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

This patch will be landing in small chunks to hopefully avoid the large reverts.


Repository:
  rL LLVM

https://reviews.llvm.org/D21675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21675: New ODR checker for modules

[Richard Trieu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL293585: Add better ODR checking for modules. (authored by 
rtrieu).

Changed prior to commit:
  https://reviews.llvm.org/D21675?vs=86142=86376#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D21675

Files:
  cfe/trunk/include/clang/AST/DeclCXX.h
  cfe/trunk/include/clang/AST/ODRHash.h
  cfe/trunk/include/clang/AST/Stmt.h
  cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td
  cfe/trunk/lib/AST/CMakeLists.txt
  cfe/trunk/lib/AST/DeclCXX.cpp
  cfe/trunk/lib/AST/ODRHash.cpp
  cfe/trunk/lib/AST/StmtProfile.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/test/Modules/merge-using-decls.cpp
  cfe/trunk/test/Modules/odr_hash.cpp

Index: cfe/trunk/lib/AST/DeclCXX.cpp
===
--- cfe/trunk/lib/AST/DeclCXX.cpp
+++ cfe/trunk/lib/AST/DeclCXX.cpp
@@ -18,6 +18,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/ODRHash.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "llvm/ADT/STLExtras.h"
@@ -71,8 +72,8 @@
   ImplicitCopyAssignmentHasConstParam(true),
   HasDeclaredCopyConstructorWithConstParam(false),
   HasDeclaredCopyAssignmentWithConstParam(false), IsLambda(false),
-  IsParsingBaseSpecifiers(false), NumBases(0), NumVBases(0), Bases(),
-  VBases(), Definition(D), FirstFriend() {}
+  IsParsingBaseSpecifiers(false), ODRHash(0), NumBases(0), NumVBases(0),
+  Bases(), VBases(), Definition(D), FirstFriend() {}
 
 CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getBasesSlowCase() const {
   return Bases.get(Definition->getASTContext().getExternalSource());
@@ -371,6 +372,16 @@
   data().IsParsingBaseSpecifiers = false;
 }
 
+void CXXRecordDecl::computeODRHash() {
+  if (!DefinitionData)
+return;
+
+  ODRHash Hash;
+  Hash.AddCXXRecordDecl(this);
+
+  DefinitionData->ODRHash = Hash.CalculateHash();
+}
+
 void CXXRecordDecl::addedClassSubobject(CXXRecordDecl *Subobj) {
   // C++11 [class.copy]p11:
   //   A defaulted copy/move constructor for a class X is defined as
Index: cfe/trunk/lib/AST/ODRHash.cpp
===
--- cfe/trunk/lib/AST/ODRHash.cpp
+++ cfe/trunk/lib/AST/ODRHash.cpp
@@ -0,0 +1,892 @@
+//===-- ODRHash.cpp - Hashing to diagnose ODR failures --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+///
+/// \file
+/// This file implements the ODRHash class, which calculates a hash based
+/// on AST nodes, which is stable across different runs.
+///
+//===--===//
+
+#include "clang/AST/ODRHash.h"
+
+#include "clang/AST/DeclVisitor.h"
+#include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/AST/TypeVisitor.h"
+
+using namespace clang;
+
+// This method adds more information that AddDecl does.
+void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
+  assert(Record && Record->hasDefinition() &&
+ "Expected non-null record to be a definition.");
+  AddDecl(Record);
+
+  // Additional information that is not needed in AddDecl.  Do not move this
+  // to ODRTypeVisitor.
+  ID.AddInteger(Record->getNumBases());
+  for (auto base : Record->bases()) {
+AddBoolean(base.isVirtual());
+AddQualType(base.getType());
+  }
+
+  const ClassTemplateDecl *TD = Record->getDescribedClassTemplate();
+  AddBoolean(TD);
+  if (TD) {
+AddTemplateParameterList(TD->getTemplateParameters());
+  }
+}
+
+// Hashing for Stmt is with Stmt::Profile, since they derive from the same base
+// class.
+void ODRHash::AddStmt(const Stmt *S) {
+  assert(S && "Expecting non-null pointer.");
+  S->ProcessODRHash(ID, *this);
+}
+
+void ODRHash::AddIdentifierInfo(const IdentifierInfo *II) {
+  assert(II && "Expecting non-null pointer.");
+  ID.AddString(II->getName());
+}
+
+void ODRHash::AddNestedNameSpecifier(const NestedNameSpecifier *NNS) {
+  assert(NNS && "Expecting non-null pointer.");
+  const auto *Prefix = NNS->getPrefix();
+  AddBoolean(Prefix);
+  if (Prefix) {
+AddNestedNameSpecifier(Prefix);
+  }
+
+  auto Kind = NNS->getKind();
+  ID.AddInteger(Kind);
+  switch (Kind) {
+  case NestedNameSpecifier::Identifier:
+AddIdentifierInfo(NNS->getAsIdentifier());
+break;
+  case NestedNameSpecifier::Namespace:
+AddDecl(NNS->getAsNamespace());
+break;
+  case NestedNameSpecifier::NamespaceAlias:
+AddDecl(NNS->getAsNamespaceAlias());
+break;
+  case 

[PATCH] D21675: New ODR checker for modules

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 86142.
rtrieu added a comment.

Changes made to the ODR hash algorithm:

Separated Decl and Type pointers into their own DenseMap's.
Removed the queue of pointers to process at the end.  Instead, process pointers 
at first use.
Save Boolean values and add them together at the end to reduce bits wasted.  
Shrinks data from bools 32x.

With these changes, the overhead is now 1-1.5%


https://reviews.llvm.org/D21675

Files:
  include/clang/AST/DeclCXX.h
  include/clang/AST/ODRHash.h
  include/clang/AST/Stmt.h
  include/clang/Basic/DiagnosticSerializationKinds.td
  lib/AST/CMakeLists.txt
  lib/AST/DeclCXX.cpp
  lib/AST/ODRHash.cpp
  lib/AST/StmtProfile.cpp
  lib/Sema/SemaDecl.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  test/Modules/merge-using-decls.cpp
  test/Modules/odr_hash.cpp

Index: test/Modules/odr_hash.cpp
===
--- test/Modules/odr_hash.cpp
+++ test/Modules/odr_hash.cpp
@@ -0,0 +1,1077 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s   >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module first {"   >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\""   >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module second {"  >> %t/Inputs/module.map
+// RUN: echo "header \"second.h\""  >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x c++ -I%t/Inputs -verify %s -std=c++11
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {
+  public:
+};
+#elif defined(SECOND)
+struct S1 {
+  private:
+};
+#else
+S1 s1;
+// expected-error@first.h:* {{'S1' has different definitions in different modules; first difference is definition in module 'first' found public access specifier}}
+// expected-note@second.h:* {{but in 'second' found private access specifier}}
+#endif
+
+#if defined(FIRST)
+struct S2Friend2 {};
+struct S2 {
+  friend S2Friend2;
+};
+#elif defined(SECOND)
+struct S2Friend1 {};
+struct S2 {
+  friend S2Friend1;
+};
+#else
+S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'first' found friend 'S2Friend2'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S2Friend1'}}
+#endif
+
+#if defined(FIRST)
+template
+struct S3Template {};
+struct S3 {
+  friend S3Template;
+};
+#elif defined(SECOND)
+template
+struct S3Template {};
+struct S3 {
+  friend S3Template;
+};
+#else
+S3 s3;
+// expected-error@first.h:* {{'S3' has different definitions in different modules; first difference is definition in module 'first' found friend 'S3Template'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S3Template'}}
+#endif
+
+#if defined(FIRST)
+struct S4 {
+  static_assert(1 == 1, "First");
+};
+#elif defined(SECOND)
+struct S4 {
+  static_assert(1 == 1, "Second");
+};
+#else
+S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'first' found static assert with message}}
+// expected-note@second.h:* {{but in 'second' found static assert with different message}}
+#endif
+
+#if defined(FIRST)
+struct S5 {
+  static_assert(1 == 1, "Message");
+};
+#elif defined(SECOND)
+struct S5 {
+  static_assert(2 == 2, "Message");
+};
+#else
+S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'first' found static assert with condition}}
+// expected-note@second.h:* {{but in 'second' found static assert with different condition}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+  int First();
+};
+#elif defined(SECOND)
+struct S6 {
+  int Second();
+};
+#else
+S6 s6;
+// expected-error@second.h:* {{'S6::Second' from module 'second' is not present in definition of 'S6' in module 'first'}}
+// expected-note@first.h:* {{definition has no member 'Second'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+  double foo();
+};
+#elif defined(SECOND)
+struct S7 {
+  int foo();
+};
+#else
+S7 s7;
+// expected-error@second.h:* {{'S7::foo' from module 'second' is not present in definition of 'S7' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'foo' does not match}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+  void foo();
+};
+#elif defined(SECOND)
+struct S8 {
+  void foo() {}
+};
+#else

[PATCH] D21675: New ODR checker for modules

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In https://reviews.llvm.org/D21675#654659, @teemperor wrote:

> I feel like we have a really similar use case in the 
> Analysis/CloneDetection{.h/.cpp} with the hashing of statements. I tried 
> substituting the call to the statement hashing with a call to the 
> CloneDetection API and it seems that most tests pass and the remaining fails 
> are just minor fixable differences (like `::foo()` and `foo()` having 
> different hash codes).
>
> Would be nice if we could make Stmt::Profile, ODRHash and the CloneDetection 
> use the same backend. We improved a few things that we no longer have the 
> periodic `realloc`s from the vector inside NodeIDSet and that we use MD5. 
> Also there are are some future plans on how we can better prevent regressions 
> when we add/extend AST nodes. Thoughts?


It would help to understand your use case better.  Does CloneDetection care 
about ::foo versus foo?  How about different types with the same name?  ODR 
checking assumes identical token sequences, so it does care about extra "::".  
ODR checking also will process information about type while CloneDetection 
looks like it only uses the type name.

I see that CloneDetector uses both llvm::MD5 and llvm::FoldingSetNodeID, and 
only adds data via StringRef.  MD5 will add the bytes as data, while 
FoldingSetNodeID will add the size of the string, then the bytes as data.  This 
means MD5 may have collisions when two strings are added back to back while 
FoldingSetNodeID will store extra data for every integer processed.  
FoldingSetNodeID doesn't have this problem if AddInteger is used.  Were the 
reallocs a big problem for CloneDetection?

I don't thinking merging Stmt::Profile, ODRHash, and CloneDetection would be a 
good idea unless the hashes needed are very similar.  Stmt::Profile and ODRHash 
already share a base for Stmt processing, which might be extendable to 
CloneDetection as well, but a full merge may be difficult.

Sadly, we don't have a good story for when an AST node gets changed with new 
properties.  The Stmt profiler does declare all the visit methods in the class 
definition, so that will catch any new Stmt nodes without a new visit method.


https://reviews.llvm.org/D21675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21675: New ODR checker for modules

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

After changing the ODRHash class a bit, the new performance numbers are 3% in 
debug and 1-1.5% in release builds.


https://reviews.llvm.org/D21675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21675: New ODR checker for modules

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked an inline comment as done.
rtrieu added a comment.

From testing, there is no difference when compiling with pre-compiled headers.  
However, when building the headers, there is a 3-4% impact on compile time.


https://reviews.llvm.org/D21675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits