[PATCH] D70571: [Coverage] Emit a gap region to cover switch bodies

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Thanks!
I'm not that familiar with the coverage counters, but this seems good as far as 
I can tell.
Maybe add the test case from PR44011? I didn't see any test cases with that 
kind of gaps between cases.


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

https://reviews.llvm.org/D70571



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

[PATCH] D70424: clang/AMDGPU: Fix default for frame-pointer attribute

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

Ping


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

https://reviews.llvm.org/D70424



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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

jdoerfert wrote:
> JonChesterfield wrote:
> > Meinersbur wrote:
> > > JonChesterfield wrote:
> > > > jdoerfert wrote:
> > > > > Meinersbur wrote:
> > > > > > jdoerfert wrote:
> > > > > > > JonChesterfield wrote:
> > > > > > > > Sharing constants between the compiler and the runtime is an 
> > > > > > > > interesting subproblem. I think the cleanest solution is the 
> > > > > > > > one used by libc, where the compiler generates header files 
> > > > > > > > containing the constants which the runtime library includes.
> > > > > > > I'd prefer not to tackle this right now but get this done first 
> > > > > > > and revisit the issue later. OK?
> > > > > > I don't think this is a good solution. It means that libomp cannot 
> > > > > > built built anymore without also building clang. Moreover, the 
> > > > > > values cannot be changed anyway since it would break the ABI.
> > > > > > 
> > > > > > I'd go the other route: The libomp defines what it's ABI is, the 
> > > > > > compiler generates code for it. 
> > > > > This patch doesn't change what we do, just where. The numbers are 
> > > > > hard coded in clang now. Let's start a discussion on the list and if 
> > > > > we come up with a different scheme we do it after this landed.
> > > > Revisit later sounds good.
> > > > 
> > > > @Meinersbur Do you know of an example of a non-llvm compiler using this 
> > > > libomp?
> > > > 
> > > > The usual order is build a compiler, then use it to build the runtime 
> > > > libraries, then the whole package can build other stuff. Provided the 
> > > > compiler doesn't need any of the runtime libraries (compiler-rt, maths 
> > > > libraries, libomp etc) itself the system bootstraps cleanly. Especially 
> > > > important when cross compiling and I suspect the gpu targets in openmp 
> > > > have similarly strict requirements on the first compiler.
> > > > 
> > > > Closely related to that, I tend to assume that the runtime libraries 
> > > > can be rewritten to best serve their only client - the associated 
> > > > compiler - so if libomp is used by out of tree compilers I'd like to 
> > > > know who we are at risk of breaking.
> > > > Do you know of an example of a non-llvm compiler using this libomp?
> > > 
> > > [[ 
> > > https://github.com/llvm-project/llvm-project/blob/master/openmp/runtime/src/kmp_gsupport.cpp
> > >  | gcc  ]] (using libomp's gomp compatibility layer), [[ 
> > > https://www.openmprtl.org/ | icc  ]] (as libomp was initially donated by 
> > > Intel).
> > > 
> > > I don't understand why it even matters if there are other compilers using 
> > > libomp. Every LLVM runtime library can be built stand-alone. 
> > > With constant values being determined during compiler bootstrapping, 
> > > programs built on one computer would be potentially ABI-incompatible with 
> > > a runtime library on another. Think about updating your 
> > > compiler-rt/libomp/libc++ on you computer causing all existing binaries 
> > > on the system to crash because constants changed in the updated 
> > > compiler's bootstrapping process.
> > > 
> > > The only use case I know that does this is are operating system's syscall 
> > > tables. Linux's reference is [[ 
> > > https://github.com/torvalds/linux/blob/master/arch/sh/include/uapi/asm/unistd_64.h
> > >  | unistd.h ]] which is platform-specific and Windows generates the table 
> > > during its [[ https://j00ru.vexillium.org/syscalls/nt/64/ | build process 
> > > ]]. Therefore on Windows, system calls can only be done through ntdll. 
> > > Even on Linux one should use the system's libc instead of directly 
> > > invoking a system call.
> > Thanks. GCC and ICC would presumably be happier with the magic numbers 
> > stored with openmp then (though with the move to a monorepo that's a little 
> > less persuasive).
> > 
> > When constants that affect the ABI change, the result won't work with 
> > existing software regardless of whether the compiler or the library 
> > contains the change. Either the new compiler builds things that don't work 
> > with the old library, or the new library doesn't work with things built by 
> > the old compiler. The two have to agree on the ABI.
> > 
> > At present, openmp does the moral equivalent of #include OpenMPKinds.def 
> > from clang. Moving the constants to libomp means clang will do the 
> > equivalent of #include OpenMPKinds.def from openmp. Breaking that 
> > dependency means making a new subproject that just holds/generates the 
> > constants, that both depend on, which seems more hassle than it's worth. 
> > 
> > I'd like to generate this header as part of the clang build (though 
> > ultimately don't care that much if it's generated as part of the openmp 
> > build) because it's going to become increasingly challenging to read as 
> > non-nvptx architectures are introduced. Likewise it 

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 230828.
ffrankies edited the summary of this revision.
ffrankies added a comment.

Implemented changes requested by @aaron.ballman

- Moved the check from the "performance" module into the new "altera" module
- Split the diagnostic message into a warning and a note
- Readability/general code refactoring changes


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

https://reviews.llvm.org/D66564

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/ClangTidyForceLinker.h
  clang-tidy/altera/AlteraTidyModule.cpp
  clang-tidy/altera/CMakeLists.txt
  clang-tidy/altera/StructPackAlignCheck.cpp
  clang-tidy/altera/StructPackAlignCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/altera-struct-pack-align.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/checkers/altera-struct-pack-align.cpp

Index: test/clang-tidy/checkers/altera-struct-pack-align.cpp
===
--- /dev/null
+++ test/clang-tidy/checkers/altera-struct-pack-align.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s altera-struct-pack-align %t -- -header-filter=.* "--" --include opencl-c.h -cl-std=CL1.2 -c
+
+// Struct needs both alignment and packing
+struct error {
+  char a;
+  double b;
+  char c;
+};
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'error' is inefficient due to padding; only needs 10 bytes but is using 24 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((packed))" to reduce the amount of padding applied to struct 'error'
+// CHECK-MESSAGES: :[[@LINE-7]]:8: warning: accessing fields in struct 'error' is inefficient due to poor alignment; currently aligned to 8 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-8]]:8: note: use "__attribute__((aligned(16)))" to align struct 'error' to 16 bytes
+
+// Struct is explicitly packed, but needs alignment
+struct error_packed {
+  char a;
+  double b;
+  char c;
+} __attribute((packed));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'error_packed' is inefficient due to poor alignment; currently aligned to 1 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'error_packed' to 16 bytes
+
+// Struct is properly packed, but needs alignment
+struct align_only {
+  char a;
+  char b;
+  char c;
+  char d;
+  int e;
+  double f;
+};
+// CHECK-MESSAGES: :[[@LINE-8]]:8: warning: accessing fields in struct 'align_only' is inefficient due to poor alignment; currently aligned to 8 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-9]]:8: note: use "__attribute__((aligned(16)))" to align struct 'align_only' to 16 bytes
+
+// Struct is perfectly packed but wrongly aligned
+struct bad_align {
+  char a;
+  double b;
+  char c;
+} __attribute((packed)) __attribute((aligned(8)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align' is inefficient due to poor alignment; currently aligned to 8 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align' to 16 bytes
+
+struct bad_align2 {
+  char a;
+  double b;
+  char c;
+} __attribute((packed)) __attribute((aligned(32)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align2' is inefficient due to poor alignment; currently aligned to 32 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align2' to 16 bytes
+
+struct bad_align3 {
+  char a;
+  double b;
+  char c;
+} __attribute((packed)) __attribute((aligned(4)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align3' is inefficient due to poor alignment; currently aligned to 4 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align3' to 16 bytes
+
+// Struct is both perfectly packed and aligned
+struct success {
+  char a;
+  double b;
+  char c;
+} __attribute((packed)) __attribute((aligned(16)));
+//Should take 10 bytes and be aligned to 16 bytes
+
+// Struct is properly packed, and explicitly aligned
+struct success2 {
+  int a;
+  int b;
+  int c;
+} __attribute((aligned(16)));
+
+// If struct is properly aligned, packing not needed
+struct error_aligned {
+  char a;
+  double b;
+  char c;
+} __attribute((aligned(16)));
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -59,6 +59,7 @@
 

[PATCH] D70575: [Clang] Define Fuchsia C++ABI

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D70575#1756072 , @leonardchan wrote:

> Could you add a test to show that with a fuchsia target we end up returning 
> `this` from constructors + destructors and ensure that this ABI is used?


Done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70575



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

[PATCH] D70575: [Clang] Define Fuchsia C++ABI

[Petr Hosek via Phabricator via cfe-commits]

phosek updated this revision to Diff 230827.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70575

Files:
  clang/include/clang/Basic/TargetCXXABI.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/constructor-destructor-return-this.cpp

Index: clang/test/CodeGenCXX/constructor-destructor-return-this.cpp
===
--- clang/test/CodeGenCXX/constructor-destructor-return-this.cpp
+++ clang/test/CodeGenCXX/constructor-destructor-return-this.cpp
@@ -3,6 +3,8 @@
 //RUN: %clang_cc1 %s -emit-llvm -o - -triple=thumbv7-apple-ios5.0 -target-abi apcs-gnu | FileCheck --check-prefix=CHECKIOS5 %s
 //RUN: %clang_cc1 %s -emit-llvm -o - -triple=wasm32-unknown-unknown \
 //RUN:   | FileCheck --check-prefix=CHECKARM %s
+//RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-unknown-fuchsia | FileCheck --check-prefix=CHECKFUCHSIA %s
+//RUN: %clang_cc1 %s -emit-llvm -o - -triple=aarch64-unknown-fuchsia | FileCheck --check-prefix=CHECKFUCHSIA %s
 //RUN: %clang_cc1 %s -emit-llvm -o - -triple=i386-pc-win32 -fno-rtti | FileCheck --check-prefix=CHECKMS %s
 // FIXME: these tests crash on the bots when run with -triple=x86_64-pc-win32
 
@@ -45,6 +47,11 @@
 // CHECKIOS5-LABEL: define %class.B* @_ZN1BD2Ev(%class.B* %this)
 // CHECKIOS5-LABEL: define %class.B* @_ZN1BD1Ev(%class.B* %this)
 
+// CHECKFUCHSIA-LABEL: define %class.B* @_ZN1BC2EPi(%class.B* returned %this, i32* %i)
+// CHECKFUCHSIA-LABEL: define %class.B* @_ZN1BC1EPi(%class.B* returned %this, i32* %i)
+// CHECKFUCHSIA-LABEL: define %class.B* @_ZN1BD2Ev(%class.B* returned %this)
+// CHECKFUCHSIA-LABEL: define %class.B* @_ZN1BD1Ev(%class.B* returned %this)
+
 // CHECKMS-LABEL: define dso_local x86_thiscallcc %class.B* @"??0B@@QAE@PAH@Z"(%class.B* returned %this, i32* %i)
 // CHECKMS-LABEL: define dso_local x86_thiscallcc void @"??1B@@UAE@XZ"(%class.B* %this)
 
@@ -83,6 +90,14 @@
 // CHECKIOS5-LABEL: define void @_ZN1CD0Ev(%class.C* %this)
 // CHECKIOS5-LABEL: define void @_ZThn8_N1CD0Ev(%class.C* %this)
 
+// CHECKFUCHSIA-LABEL: define %class.C* @_ZN1CC2EPiPc(%class.C* returned %this, i32* %i, i8* %c)
+// CHECKFUCHSIA-LABEL: define %class.C* @_ZN1CC1EPiPc(%class.C* returned %this, i32* %i, i8* %c)
+// CHECKFUCHSIA-LABEL: define %class.C* @_ZN1CD2Ev(%class.C* returned %this)
+// CHECKFUCHSIA-LABEL: define %class.C* @_ZN1CD1Ev(%class.C* returned %this)
+// CHECKFUCHSIA-LABEL: define %class.C* @_ZThn16_N1CD1Ev(%class.C* %this)
+// CHECKFUCHSIA-LABEL: define void @_ZN1CD0Ev(%class.C* %this)
+// CHECKFUCHSIA-LABEL: define void @_ZThn16_N1CD0Ev(%class.C* %this)
+
 // CHECKMS-LABEL: define dso_local x86_thiscallcc %class.C* @"??0C@@QAE@PAHPAD@Z"(%class.C* returned %this, i32* %i, i8* %c)
 // CHECKMS-LABEL: define dso_local x86_thiscallcc void @"??1C@@UAE@XZ"(%class.C* %this)
 
@@ -110,6 +125,11 @@
 // CHECKIOS5-LABEL: define %class.D* @_ZN1DD2Ev(%class.D* %this, i8** %vtt)
 // CHECKIOS5-LABEL: define %class.D* @_ZN1DD1Ev(%class.D* %this)
 
+// CHECKFUCHSIA-LABEL: define %class.D* @_ZN1DC2Ev(%class.D* returned %this, i8** %vtt)
+// CHECKFUCHSIA-LABEL: define %class.D* @_ZN1DC1Ev(%class.D* returned %this)
+// CHECKFUCHSIA-LABEL: define %class.D* @_ZN1DD2Ev(%class.D* returned %this, i8** %vtt)
+// CHECKFUCHSIA-LABEL: define %class.D* @_ZN1DD1Ev(%class.D* returned %this)
+
 // CHECKMS-LABEL: define dso_local x86_thiscallcc %class.D* @"??0D@@QAE@XZ"(%class.D* returned %this, i32 %is_most_derived)
 // CHECKMS-LABEL: define dso_local x86_thiscallcc void @"??1D@@UAE@XZ"(%class.D* %this)
 
@@ -139,3 +159,16 @@
 // CHECKARM: {{%.*}} = call %class.E* @_ZN1ED1Ev(%class.E* %
 
 // CHECKARM: declare %class.E* @_ZN1ED1Ev(%class.E* returned)
+
+// CHECKFUCHSIA_LABEL: define void @_Z15test_destructorv()
+
+// Verify that virtual calls to destructors are not marked with a 'returned'
+// this parameter at the call site...
+// CHECKFUCHSIA: [[VFN:%.*]] = getelementptr inbounds %class.E* (%class.E*)*, %class.E* (%class.E*)**
+// CHECKFUCHSIA: [[THUNK:%.*]] = load %class.E* (%class.E*)*, %class.E* (%class.E*)** [[VFN]]
+// CHECKFUCHSIA: call %class.E* [[THUNK]](%class.E* %
+
+// ...but static calls create declarations with 'returned' this
+// CHECKFUCHSIA: {{%.*}} = call %class.E* @_ZN1ED1Ev(%class.E* %
+
+// CHECKFUCHSIA: declare %class.E* @_ZN1ED1Ev(%class.E* returned)
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -487,6 +487,19 @@
   bool shouldRTTIBeUnique() const override { return false; }
 };
 
+class FuchsiaCXXABI final : public ItaniumCXXABI {
+public:
+  explicit FuchsiaCXXABI(CodeGen::CodeGenModule )
+  : ItaniumCXXABI(CGM) {}
+
+private:
+  bool HasThisReturn(GlobalDecl GD) const override {

[PATCH] D69876: Allow output constraints on "asm goto"

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Any further comments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69876



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

[PATCH] D70624: [Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

[Dávid Bolvanský via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGba4017670e19: [Diagnostics] Warn for comparison with string 
literals expanded from macro… (authored by xbolva00).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70624

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/exprs.c
  clang/test/Sema/warn-stringcompare.c

Index: clang/test/Sema/warn-stringcompare.c
===
--- /dev/null
+++ clang/test/Sema/warn-stringcompare.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+#define DELIM "/"
+#define DOT "."
+#define NULL (void *)0
+
+void test(const char *d) {
+  if ("/" != d) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if (d == "/") // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if ("/" != NULL)
+return;
+  if (NULL == "/")
+return;
+  if ("/" != DELIM) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if (DELIM == "/") // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if (DELIM != NULL)
+return;
+  if (NULL == DELIM)
+return;
+  if (DOT != DELIM) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if (DELIM == DOT) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+}
Index: clang/test/Sema/exprs.c
===
--- clang/test/Sema/exprs.c
+++ clang/test/Sema/exprs.c
@@ -119,7 +119,7 @@
 
 // PR3753
 int test12(const char *X) {
-  return X == "foo";  // expected-warning {{comparison against a string literal is unspecified (use strncmp instead)}}
+  return X == "foo";  // expected-warning {{comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
 }
 
 int test12b(const char *X) {
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10338,7 +10338,6 @@
   QualType RHSType = RHS->getType();
   if (LHSType->hasFloatingRepresentation() ||
   (LHSType->isBlockPointerType() && !BinaryOperator::isEqualityOp(Opc)) ||
-  LHS->getBeginLoc().isMacroID() || RHS->getBeginLoc().isMacroID() ||
   S.inTemplateInstantiation())
 return;
 
@@ -10366,45 +10365,51 @@
 AlwaysEqual, // std::strong_ordering::equal from operator<=>
   };
 
-  if (Expr::isSameComparisonOperand(LHS, RHS)) {
-unsigned Result;
-switch (Opc) {
-case BO_EQ: case BO_LE: case BO_GE:
-  Result = AlwaysTrue;
-  break;
-case BO_NE: case BO_LT: case BO_GT:
-  Result = AlwaysFalse;
-  break;
-case BO_Cmp:
-  Result = AlwaysEqual;
-  break;
-default:
-  Result = AlwaysConstant;
-  break;
-}
-S.DiagRuntimeBehavior(Loc, nullptr,
-  S.PDiag(diag::warn_comparison_always)
-  << 0 /*self-comparison*/
-  << Result);
-  } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
-// What is it always going to evaluate to?
-unsigned Result;
-switch(Opc) {
-case BO_EQ: // e.g. array1 == array2
-  Result = AlwaysFalse;
-  break;
-case BO_NE: // e.g. array1 != array2
-  Result = AlwaysTrue;
-  break;
-default: // e.g. array1 <= array2
-  // The best we can say is 'a constant'
-  Result = AlwaysConstant;
-  break;
+  if (!LHS->getBeginLoc().isMacroID() && !RHS->getBeginLoc().isMacroID()) {
+if (Expr::isSameComparisonOperand(LHS, RHS)) {
+  unsigned Result;
+  switch (Opc) {
+  case BO_EQ:
+  case BO_LE:
+  case BO_GE:
+Result = AlwaysTrue;
+break;
+  case BO_NE:
+  case BO_LT:
+  case BO_GT:
+Result = AlwaysFalse;
+break;
+  case BO_Cmp:
+Result = AlwaysEqual;
+break;
+  default:
+Result = AlwaysConstant;
+break;
+  }
+  S.DiagRuntimeBehavior(Loc, nullptr,
+S.PDiag(diag::warn_comparison_always)
+<< 0 /*self-comparison*/
+<< Result);
+} else if (checkForArray(LHSStripped) && 

[PATCH] D70624: [Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Thank you for the review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70624



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

[PATCH] D70624: [Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman 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/D70624/new/

https://reviews.llvm.org/D70624



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

[PATCH] D70624: [Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 230797.
xbolva00 added a comment.

Added new tests.
Improved recommendation how to fix warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70624

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/exprs.c
  clang/test/Sema/warn-stringcompare.c

Index: clang/test/Sema/warn-stringcompare.c
===
--- /dev/null
+++ clang/test/Sema/warn-stringcompare.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+#define DELIM "/"
+#define DOT "."
+#define NULL (void *)0
+
+void test(const char *d) {
+  if ("/" != d) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if (d == "/") // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if ("/" != NULL)
+return;
+  if (NULL == "/")
+return;
+  if ("/" != DELIM) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if (DELIM == "/") // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if (DELIM != NULL)
+return;
+  if (NULL == DELIM)
+return;
+  if (DOT != DELIM) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+  if (DELIM == DOT) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
+return;
+}
Index: clang/test/Sema/exprs.c
===
--- clang/test/Sema/exprs.c
+++ clang/test/Sema/exprs.c
@@ -119,7 +119,7 @@
 
 // PR3753
 int test12(const char *X) {
-  return X == "foo";  // expected-warning {{comparison against a string literal is unspecified (use strncmp instead)}}
+  return X == "foo";  // expected-warning {{comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
 }
 
 int test12b(const char *X) {
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10338,7 +10338,6 @@
   QualType RHSType = RHS->getType();
   if (LHSType->hasFloatingRepresentation() ||
   (LHSType->isBlockPointerType() && !BinaryOperator::isEqualityOp(Opc)) ||
-  LHS->getBeginLoc().isMacroID() || RHS->getBeginLoc().isMacroID() ||
   S.inTemplateInstantiation())
 return;
 
@@ -10366,45 +10365,51 @@
 AlwaysEqual, // std::strong_ordering::equal from operator<=>
   };
 
-  if (Expr::isSameComparisonOperand(LHS, RHS)) {
-unsigned Result;
-switch (Opc) {
-case BO_EQ: case BO_LE: case BO_GE:
-  Result = AlwaysTrue;
-  break;
-case BO_NE: case BO_LT: case BO_GT:
-  Result = AlwaysFalse;
-  break;
-case BO_Cmp:
-  Result = AlwaysEqual;
-  break;
-default:
-  Result = AlwaysConstant;
-  break;
-}
-S.DiagRuntimeBehavior(Loc, nullptr,
-  S.PDiag(diag::warn_comparison_always)
-  << 0 /*self-comparison*/
-  << Result);
-  } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
-// What is it always going to evaluate to?
-unsigned Result;
-switch(Opc) {
-case BO_EQ: // e.g. array1 == array2
-  Result = AlwaysFalse;
-  break;
-case BO_NE: // e.g. array1 != array2
-  Result = AlwaysTrue;
-  break;
-default: // e.g. array1 <= array2
-  // The best we can say is 'a constant'
-  Result = AlwaysConstant;
-  break;
+  if (!LHS->getBeginLoc().isMacroID() && !RHS->getBeginLoc().isMacroID()) {
+if (Expr::isSameComparisonOperand(LHS, RHS)) {
+  unsigned Result;
+  switch (Opc) {
+  case BO_EQ:
+  case BO_LE:
+  case BO_GE:
+Result = AlwaysTrue;
+break;
+  case BO_NE:
+  case BO_LT:
+  case BO_GT:
+Result = AlwaysFalse;
+break;
+  case BO_Cmp:
+Result = AlwaysEqual;
+break;
+  default:
+Result = AlwaysConstant;
+break;
+  }
+  S.DiagRuntimeBehavior(Loc, nullptr,
+S.PDiag(diag::warn_comparison_always)
+<< 0 /*self-comparison*/
+<< Result);
+} else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
+  // What is it always going to evaluate to?
+ 

[PATCH] D70624: [Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:10368
 
-  if (Expr::isSameComparisonOperand(LHS, RHS)) {
-unsigned Result;
-switch (Opc) {
-case BO_EQ: case BO_LE: case BO_GE:
-  Result = AlwaysTrue;
-  break;
-case BO_NE: case BO_LT: case BO_GT:
-  Result = AlwaysFalse;
-  break;
-case BO_Cmp:
-  Result = AlwaysEqual;
-  break;
-default:
-  Result = AlwaysConstant;
-  break;
-}
-S.DiagRuntimeBehavior(Loc, nullptr,
-  S.PDiag(diag::warn_comparison_always)
-  << 0 /*self-comparison*/
-  << Result);
-  } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
-// What is it always going to evaluate to?
-unsigned Result;
-switch(Opc) {
-case BO_EQ: // e.g. array1 == array2
-  Result = AlwaysFalse;
-  break;
-case BO_NE: // e.g. array1 != array2
-  Result = AlwaysTrue;
-  break;
-default: // e.g. array1 <= array2
-  // The best we can say is 'a constant'
-  Result = AlwaysConstant;
-  break;
+  if (!LHS->getBeginLoc().isMacroID() && !RHS->getBeginLoc().isMacroID()) {
+if (Expr::isSameComparisonOperand(LHS, RHS)) {

xbolva00 wrote:
> aaron.ballman wrote:
> > Why do we care about the macros here? It seems just as reasonable to warn 
> > on:
> > ```
> > #define MACRO1 "one"
> > #define MACRO2 "one
> > 
> > if (MACRO1 == MACRO2) // Why not warn here too?
> > ```
> > 
> This “macro loc” check is for array comparison diagnostic. I preserved 
> original logic of this function and enabled -Wstring-compare for macro 
> locations.
> 
> We warn here (I will add a test).
Ah, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70624



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

[PATCH] D70071: [ConstExprPreter] Removed the flag forcing the use of the interpreter

[Nandor Licker via Phabricator via cfe-commits]

nand updated this revision to Diff 230791.
nand added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70071

Files:
  clang/docs/ConstantInterpreter.rst
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Context.cpp
  clang/lib/AST/Interp/Context.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/AST/Interp/cond.cpp

Index: clang/test/AST/Interp/cond.cpp
===
--- clang/test/AST/Interp/cond.cpp
+++ clang/test/AST/Interp/cond.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fforce-experimental-new-constant-interpreter %s -verify
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fexperimental-new-constant-interpreter %s -verify
 // RUN: %clang_cc1 -std=c++17 -fsyntax-only %s -verify
 // expected-no-diagnostics
 
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2854,8 +2854,6 @@
   getLastArgIntValue(Args, OPT_fconstexpr_steps, 1048576, Diags);
   Opts.EnableNewConstInterp =
   Args.hasArg(OPT_fexperimental_new_constant_interpreter);
-  Opts.ForceNewConstInterp =
-  Args.hasArg(OPT_fforce_experimental_new_constant_interpreter);
   Opts.BracketDepth = getLastArgIntValue(Args, OPT_fbracket_depth, 256, Diags);
   Opts.DelayedTemplateParsing = Args.hasArg(OPT_fdelayed_template_parsing);
   Opts.NumLargeByValueCopy =
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4484,9 +4484,6 @@
   if (Args.hasArg(options::OPT_fexperimental_new_constant_interpreter))
 CmdArgs.push_back("-fexperimental-new-constant-interpreter");
 
-  if (Args.hasArg(options::OPT_fforce_experimental_new_constant_interpreter))
-CmdArgs.push_back("-fforce-experimental-new-constant-interpreter");
-
   if (Arg *A = Args.getLastArg(options::OPT_fbracket_depth_EQ)) {
 CmdArgs.push_back("-fbracket-depth");
 CmdArgs.push_back(A->getValue());
Index: clang/lib/AST/Interp/Context.h
===
--- clang/lib/AST/Interp/Context.h
+++ clang/lib/AST/Interp/Context.h
@@ -34,16 +34,6 @@
 class State;
 enum PrimType : unsigned;
 
-/// Wrapper around interpreter termination results.
-enum class InterpResult {
-  /// Interpreter successfully computed a value.
-  Success,
-  /// Interpreter encountered an error and quit.
-  Fail,
-  /// Interpreter encountered an unimplemented feature, AST fallback.
-  Bail,
-};
-
 /// Holds all information required to evaluate constexpr code in a module.
 class Context {
 public:
@@ -54,15 +44,13 @@
   ~Context();
 
   /// Checks if a function is a potential constant expression.
-  InterpResult isPotentialConstantExpr(State ,
-   const FunctionDecl *FnDecl);
+  bool isPotentialConstantExpr(State , const FunctionDecl *FnDecl);
 
   /// Evaluates a toplevel expression as an rvalue.
-  InterpResult evaluateAsRValue(State , const Expr *E, APValue );
+  bool evaluateAsRValue(State , const Expr *E, APValue );
 
   /// Evaluates a toplevel initializer.
-  InterpResult evaluateAsInitializer(State , const VarDecl *VD,
- APValue );
+  bool evaluateAsInitializer(State , const VarDecl *VD, APValue );
 
   /// Returns the AST context.
   ASTContext () const { return Ctx; }
@@ -78,16 +66,14 @@
 
 private:
   /// Runs a function.
-  InterpResult Run(State , Function *Func, APValue );
+  bool Run(State , Function *Func, APValue );
 
   /// Checks a result fromt the interpreter.
-  InterpResult Check(State , llvm::Expected &);
+  bool Check(State , llvm::Expected &);
 
 private:
   /// Current compilation context.
   ASTContext 
-  /// Flag to indicate if the use of the interpreter is mandatory.
-  bool ForceInterp;
   /// Interpreter stack, shared across invocations.
   InterpStack Stk;
   /// Constexpr program.
Index: clang/lib/AST/Interp/Context.cpp
===
--- clang/lib/AST/Interp/Context.cpp
+++ clang/lib/AST/Interp/Context.cpp
@@ -21,44 +21,37 @@
 using namespace clang;
 using namespace clang::interp;
 
-Context::Context(ASTContext )
-: Ctx(Ctx), ForceInterp(getLangOpts().ForceNewConstInterp),
-  P(new Program(*this)) {}
+Context::Context(ASTContext ) : Ctx(Ctx), P(new Program(*this)) {}
 
 Context::~Context() {}
 
-InterpResult Context::isPotentialConstantExpr(State ,
-  const FunctionDecl *FD) {
+bool Context::isPotentialConstantExpr(State , const 

[PATCH] D70190: [clang][modules] Add support for merging lifetime-extended temporaries

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: 
clang/test/Modules/Inputs/merge-lifetime-extended-temporary/module.modulemap:15
+}
\ No newline at end of file


Add newline here.

Dtto in other tests above.


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

https://reviews.llvm.org/D70190



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

[PATCH] D70624: [Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

[Dávid Bolvanský via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8431
   "result of comparison against %select{a string literal|@encode}0 is "
-  "unspecified (use strncmp instead)">,
-  InGroup;
+  "unspecified">, InGroup;
 

aaron.ballman wrote:
> I think this is a regression in the diagnostic because we dropped the 
> recommendation for how to fix the issue. Why do we need to drop that? I'd be 
> fine if it was made less specific, like: `(use an explicit string comparison 
> function instead)` if the concern is over `strncmp` specifically.
Yes, my concern was about strncmp. Your suggestion is fine.



Comment at: clang/lib/Sema/SemaExpr.cpp:10368
 
-  if (Expr::isSameComparisonOperand(LHS, RHS)) {
-unsigned Result;
-switch (Opc) {
-case BO_EQ: case BO_LE: case BO_GE:
-  Result = AlwaysTrue;
-  break;
-case BO_NE: case BO_LT: case BO_GT:
-  Result = AlwaysFalse;
-  break;
-case BO_Cmp:
-  Result = AlwaysEqual;
-  break;
-default:
-  Result = AlwaysConstant;
-  break;
-}
-S.DiagRuntimeBehavior(Loc, nullptr,
-  S.PDiag(diag::warn_comparison_always)
-  << 0 /*self-comparison*/
-  << Result);
-  } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
-// What is it always going to evaluate to?
-unsigned Result;
-switch(Opc) {
-case BO_EQ: // e.g. array1 == array2
-  Result = AlwaysFalse;
-  break;
-case BO_NE: // e.g. array1 != array2
-  Result = AlwaysTrue;
-  break;
-default: // e.g. array1 <= array2
-  // The best we can say is 'a constant'
-  Result = AlwaysConstant;
-  break;
+  if (!LHS->getBeginLoc().isMacroID() && !RHS->getBeginLoc().isMacroID()) {
+if (Expr::isSameComparisonOperand(LHS, RHS)) {

aaron.ballman wrote:
> Why do we care about the macros here? It seems just as reasonable to warn on:
> ```
> #define MACRO1 "one"
> #define MACRO2 "one
> 
> if (MACRO1 == MACRO2) // Why not warn here too?
> ```
> 
This “macro loc” check is for array comparison diagnostic. I preserved original 
logic of this function and enabled -Wstring-compare for macro locations.

We warn here (I will add a test).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70624



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

[PATCH] D70190: [clang][modules] Add support for merging lifetime-extended temporaries

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 230790.
Tyker added a comment.

rebased
added new lines.


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

https://reviews.llvm.org/D70190

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/merge-lifetime-extended-temporary/a.h
  clang/test/Modules/Inputs/merge-lifetime-extended-temporary/b.h
  clang/test/Modules/Inputs/merge-lifetime-extended-temporary/c.h
  clang/test/Modules/Inputs/merge-lifetime-extended-temporary/module.modulemap
  clang/test/Modules/merge-lifetime-extended-temporary.cpp

Index: clang/test/Modules/merge-lifetime-extended-temporary.cpp
===
--- /dev/null
+++ clang/test/Modules/merge-lifetime-extended-temporary.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x c++ -I%S/Inputs/merge-lifetime-extended-temporary -verify -std=c++11 %s -DORDER=1
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x c++ -I%S/Inputs/merge-lifetime-extended-temporary -verify -std=c++11 %s -DORDER=2
+
+// expected-no-diagnostics
+#if ORDER == 1
+#include "c.h"
+#include "b.h"
+#else
+#include "b.h"
+#include "c.h"
+#endif
+
+static_assert(PtrTemp1 == , "");
+static_assert(PtrTemp1 == PtrTemp2, "");
Index: clang/test/Modules/Inputs/merge-lifetime-extended-temporary/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/merge-lifetime-extended-temporary/module.modulemap
@@ -0,0 +1,14 @@
+module "a" {
+  export *
+  header "a.h"
+}
+
+module "b" {
+  export *
+  header "b.h"
+}
+
+module "c" {
+  export *
+  header "c.h"
+}
Index: clang/test/Modules/Inputs/merge-lifetime-extended-temporary/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/merge-lifetime-extended-temporary/c.h
@@ -0,0 +1,4 @@
+
+#include "a.h"
+
+constexpr const int* PtrTemp2 = 
Index: clang/test/Modules/Inputs/merge-lifetime-extended-temporary/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/merge-lifetime-extended-temporary/b.h
@@ -0,0 +1,4 @@
+
+#include "a.h"
+
+constexpr const int* PtrTemp1 = 
Index: clang/test/Modules/Inputs/merge-lifetime-extended-temporary/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/merge-lifetime-extended-temporary/a.h
@@ -0,0 +1,2 @@
+
+constexpr const int& LETemp = 0;
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -424,6 +424,9 @@
 template
 void mergeMergeable(Mergeable *D);
 
+template <>
+void mergeMergeable(Mergeable *D);
+
 void mergeTemplatePattern(RedeclarableTemplateDecl *D,
   RedeclarableTemplateDecl *Existing,
   DeclID DsID, bool IsKeyDecl);
@@ -2358,6 +2361,7 @@
   if (Record.readInt())
 D->Value = new (D->getASTContext()) APValue(Record.readAPValue());
   D->ManglingNumber = Record.readInt();
+  mergeMergeable(D);
 }
 
 std::pair
@@ -2555,6 +2559,28 @@
   return false;
 }
 
+/// Attempts to merge LifetimeExtendedTemporaryDecl with
+/// identical class definitions from two different modules.
+template<>
+void ASTDeclReader::mergeMergeable(
+Mergeable *D) {
+  // If modules are not available, there is no reason to perform this merge.
+  if (!Reader.getContext().getLangOpts().Modules)
+return;
+
+  LifetimeExtendedTemporaryDecl *LETDecl =
+  static_cast(D);
+
+  LifetimeExtendedTemporaryDecl * =
+  Reader.LETemporaryForMerging[std::make_pair(
+  LETDecl->getExtendingDecl(), LETDecl->getManglingNumber())];
+  if (LookupResult)
+Reader.getContext().setPrimaryMergedDecl(LETDecl,
+ LookupResult->getCanonicalDecl());
+  else
+LookupResult = LETDecl;
+}
+
 /// Attempts to merge the given declaration (D) with another declaration
 /// of the same entity, for the case where the entity is not actually
 /// redeclarable. This happens, for instance, when merging the fields of
Index: clang/lib/AST/TextNodeDumper.cpp
===
--- clang/lib/AST/TextNodeDumper.cpp
+++ clang/lib/AST/TextNodeDumper.cpp
@@ -1338,6 +1338,17 @@
 OS << " <>>";
 }
 
+void TextNodeDumper::VisitLifetimeExtendedTemporaryDecl(
+const LifetimeExtendedTemporaryDecl *D) {
+  OS << " extended by ";
+  dumpBareDeclRef(D->getExtendingDecl());
+  OS << " mangling ";
+  {
+ColorScope Color(OS, ShowColors, 

[PATCH] D70624: [Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8431
   "result of comparison against %select{a string literal|@encode}0 is "
-  "unspecified (use strncmp instead)">,
-  InGroup;
+  "unspecified">, InGroup;
 

I think this is a regression in the diagnostic because we dropped the 
recommendation for how to fix the issue. Why do we need to drop that? I'd be 
fine if it was made less specific, like: `(use an explicit string comparison 
function instead)` if the concern is over `strncmp` specifically.



Comment at: clang/lib/Sema/SemaExpr.cpp:10368
 
-  if (Expr::isSameComparisonOperand(LHS, RHS)) {
-unsigned Result;
-switch (Opc) {
-case BO_EQ: case BO_LE: case BO_GE:
-  Result = AlwaysTrue;
-  break;
-case BO_NE: case BO_LT: case BO_GT:
-  Result = AlwaysFalse;
-  break;
-case BO_Cmp:
-  Result = AlwaysEqual;
-  break;
-default:
-  Result = AlwaysConstant;
-  break;
-}
-S.DiagRuntimeBehavior(Loc, nullptr,
-  S.PDiag(diag::warn_comparison_always)
-  << 0 /*self-comparison*/
-  << Result);
-  } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
-// What is it always going to evaluate to?
-unsigned Result;
-switch(Opc) {
-case BO_EQ: // e.g. array1 == array2
-  Result = AlwaysFalse;
-  break;
-case BO_NE: // e.g. array1 != array2
-  Result = AlwaysTrue;
-  break;
-default: // e.g. array1 <= array2
-  // The best we can say is 'a constant'
-  Result = AlwaysConstant;
-  break;
+  if (!LHS->getBeginLoc().isMacroID() && !RHS->getBeginLoc().isMacroID()) {
+if (Expr::isSameComparisonOperand(LHS, RHS)) {

Why do we care about the macros here? It seems just as reasonable to warn on:
```
#define MACRO1 "one"
#define MACRO2 "one

if (MACRO1 == MACRO2) // Why not warn here too?
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70624



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:30-31
+static bool isValueType(const Type *T) {
+  return !(isa(T) || isa(T) || isa(T) ||
+   isa(T));
 }

ObjC pointer types?



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:34-41
+static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); 
}
+static bool isReferenceType(QualType QT) {
+  return isa(QT.getTypePtr());
+}
+static bool isPointerType(const Type *T) { return isa(T); }
+static bool isPointerType(QualType QT) {
+  return isPointerType(QT.getTypePtr());

I don't see how these are improvements over checking `QT->isArrayType()` and 
friends within the caller.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:82
+return (llvm::Twine(' ') + DeclSpec::getSpecifierName(Qualifier)).str();
+  return (llvm::Twine(DeclSpec::getSpecifierName(Qualifier)) + llvm::Twine(' 
'))
+  .str();

Do you need both casts for `Twine`? I would imagine only the first is needed.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:166
+
+  llvm_unreachable("All paths should have been handled");
+}

I think this path is reachable -- it should be an `assert()` instead and return 
`None`.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:192
+  QualifierPolicy QualPolicy,
+  const ASTContext *Context) {
+  assert((QualPolicy == QualifierPolicy::Left ||

I think `Context` should be by reference instead of by pointer.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:228-229
+
+  llvm_unreachable(
+  "All possible combinations should have been handled already");
+}

I think this is reachable as well.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:27
 
+/// This enum defines where the 'const' shall be preferably added.
+enum class QualifierPolicy {

const -> qualifier (similar for the comments in the enumerators)



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:33
+
+/// This enum defines which entity is the target for adding the 'const'. This
+/// makes only a difference for pointer-types. Other types behave identical

Same here.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:37
+enum class QualifierTarget {
+  Pointee, /// Transforming a pointer goes for the pointee and not the pointer
+   /// itself. For references and normal values this option has no

goes for -> attaches to?



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:45
+
+/// \brief Creates fix to make ``VarDecl`` const qualified. Only valid if
+/// `Var` is isolated in written code. `int foo = 42;`

Comment seems out of date here as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D68912: Adds -Wrange-loop-analysis to -Wall

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

In D68912#1757850 , @xbolva00 wrote:

> In D68912#1754660 , @xbolva00 wrote:
>
> > I think you can commit it, you already fixed some warnings.
>
>
> Oh, I hit this (-Werror bots) hard :D Some bots use -Werror. So when build is 
> warning-free, then please commit this patch.


I will, a bit busy at the moment but will look at fixing the warnings before 
committing.


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

https://reviews.llvm.org/D68912



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

[PATCH] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

[Chandler Carruth via Phabricator via cfe-commits]

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

Minor nits around redundant predicates for SROA. With thouse fixed, LGTM.

I'd really love to find a way to make TCO debuggable so that we don't lose 
that. I'm particularly worried about code that relies on it to not run out of 
stack. Not sure what the best thing to do here is though. Anyways, not relevant 
for this iteration. I mostly feel bad for a potential future re-churn of all 
the tests. ;]




Comment at: llvm/lib/Passes/PassBuilder.cpp:395
   // scalars.
-  FPM.addPass(SROA());
+  if (Level >= O1)
+FPM.addPass(SROA());

We know `O0` isn't used here, so this should be a no-op.



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:263
   FPM.add(createCFGSimplificationPass());
-  FPM.add(createSROAPass());
+  if (OptLevel >= 1)
+FPM.add(createSROAPass());

We early exit at `O0` above, so this is a no-op.



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:294
 MPM.add(createFunctionInliningPass(IP));
-MPM.add(createSROAPass());
+if (OptLevel >= 1)
+  MPM.add(createSROAPass());

We only reach here if `OptLevel > 0` so this should be redundant?



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:325
   // Break up aggregate allocas, using SSAUpdater.
-  MPM.add(createSROAPass());
+  if (OptLevel >= 1)
+MPM.add(createSROAPass());

This doesn't have the assert, but I believe this is only used above `O0` as 
well. Maybe just add the assert?



Comment at: llvm/test/Other/new-pm-defaults.ll:253
+; CHECK-Os-NEXT: Finished llvm::Function pass manager run.
+; CHECK-Oz-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-EP-SCALAR-LATE-NEXT: Running pass: NoOpFunctionPass

mehdi_amini wrote:
> Just a drive-by idea: you could have simplified the changes here with a new 
> prefix: "CHECK-O23sz" that you could add to the non-O1 invocations.
> 
Good idea!


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

https://reviews.llvm.org/D65410



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

(Just a reminder that we need to have both performance and code size numbers 
for this patch. And given that there are a few options, may need a few 
examples.)


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

https://reviews.llvm.org/D70157



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