[PATCH] D70568: [Support] Possibly use exception handler in the Crash Recovery Context in the same way as global exceptions

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:269
+  const_cast(CRCI)->HandleCrash(
+  (int)ExceptionInfo->ExceptionRecord->ExceptionCode, ExceptionInfo);
 

This implicitly converts a pointer to `uintptr_t`, which is an error by default 
in mingw mode; a cast is needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70568



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

[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D72304#1815793 , @fghanim wrote:

> > Commits need a message that explains the change.
> >  Also the "Summary" should not be in the commit title.
>
> Is this something for future commits or do you want me to change these?
>  If the latter, how do I do that?


Already for this one, at least the git commit.
You can edit this patch in phab (top right, edit revision) and you need to make 
sure to adjust the git commit before pushing.

>> This adds support for clang to build the directives via the OMPIRBuilder so 
>> we need to add tests for that. Probably "just" running existing codegen 
>> tests for the directives with the OMPIRBuidlder enabled.
> 
> You mean unit tests? or something else? If not unit tests, Where do I do that?

We can use lit test for that. The two files that you need to modify for sure 
are `clang/test/OpenMP/critical_codegen.cpp` and 
`clang/test/OpenMP/master_codegen.cpp`. You add run lines so that the code in 
clang is actually executed and modify the check lines appropriately.
I did split my commits into the llvm and clang part mostly, if you look at a 
clang patch which uses the OpenMPIRBuilder, you should see what I mean.

> As for rest of comments, anything without response, is something I will do.

I did respond to the ones with.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020
+  if (AllocaIP.isSet())
+AllocaInsertPt = &*AllocaIP.getPoint();
+  auto OldReturnBlock = ReturnBlock;

fghanim wrote:
> jdoerfert wrote:
> > Why the `isSet` check? If we need it, then it's missing in other places as 
> > well (I think).
> While I was writing the response I realized that I should've made it
> ` assert((!AllocaIP.isSet() || AllocaInsertPt == AllocaIP.getpoint()) && 
> "Inlined directives allocas should be emitted to entry block of the inner 
> most containing outlined function"; ` 
> or something less verbose.
> 
> Inlined directives should emit their `alloca`s to the entry block of the 
> containing outined function, right? For that reason, and since we don't store 
> an `alloca` insertion point in the OMPBuilder, I pass an empty insertion 
> point, and back up the current point, without changing it.
> I was planning, in the future, to keep a copy of the most recent alloca 
> insertion point which we set when we outline, and pass that for `BodyCB`s 
> within inlined directives.
I try not to make assumptions like "Inlined directives should emit their 
allocas to the entry block of the containing outined function, right" if we 
don't need to. If we do, the assert is probably a better way to go. I would 
actually say we want a helper that sets/resets all these things for *all* body 
functions. If the OMPBuilder doesn't want to set a new allocaip, it should 
provide an "invalid" location to indicate that. Now I finally see what you do 
here and why. Go with the assert for now, and make it `!isSet()` if there is no 
reason to complicate it further.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3076
+  EmitBranchThroughCleanup(Dest);
+};
+

fghanim wrote:
> jdoerfert wrote:
> > I somehow have the feeling we could have a "FiniCB" helper function as they 
> > seem to be always the same. A TODO mentioning that is also OK for now.
> I'll do a Todo for now. I am using a very similar `FiniCB` to the one you 
> used for `parallel`. So it doesn't make sense to change these here and not 
> that one with them.
Agreed. We need a helper that is used at both/all places.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3099
+  ReturnBlock = OldReturnBlock;
+};
+

fghanim wrote:
> jdoerfert wrote:
> > Same here, that looks like the other "BodyGenCB". We should really make 
> > them helpers. 
> Same as prev.
Again a TODO and we are good for now.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:68
+  // Type::getInt32Ty(M.getContext()),/*NumElements*/ 8);
+  //  KmpCriticalNamePtrTy = PointerType::getUnqual(KmpCriticalNameTy);
   // Create all simple and struct types exposed by the runtime and remember

fghanim wrote:
> jdoerfert wrote:
> > These comments that reference `KmpCriticalNameTy` do not help here. If you 
> > want, feel free to add comments that explain how the result looks in a 
> > generic way but only showing `KmpCriticalNameTy` seems counterproductive to 
> > me.
> I was testing something, and I forgot to remove them when I was done.
I take it you'll remove them. Just saying because you commented ;)



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:717
+  // emit exit call and do any needed finalization.
+  auto FinIP = InsertPointTy(FiniBB, FiniBB->getFirstInsertionPt());
+  assert(FiniBB->getTerminator()->getNumSuccessors() == 1 &&

fghanim wrote:
> jdoerfert wrote:
> > Nit: `InsertPointTy 

[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

[Fady Ghanim via Phabricator via cfe-commits]

fghanim marked 10 inline comments as done.
fghanim added a comment.

> Commits need a message that explains the change.
>  Also the "Summary" should not be in the commit title.

Is this something for future commits or do you want me to change these?
If the latter, how do I do that?

> This adds support for clang to build the directives via the OMPIRBuilder so 
> we need to add tests for that. Probably "just" running existing codegen tests 
> for the directives with the OMPIRBuidlder enabled.

You mean unit tests? or something else? If not unit tests, Where do I do that?

As for rest of comments, anything without response, is something I will do.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020
+  if (AllocaIP.isSet())
+AllocaInsertPt = &*AllocaIP.getPoint();
+  auto OldReturnBlock = ReturnBlock;

jdoerfert wrote:
> Why the `isSet` check? If we need it, then it's missing in other places as 
> well (I think).
While I was writing the response I realized that I should've made it
` assert((!AllocaIP.isSet() || AllocaInsertPt == AllocaIP.getpoint()) && 
"Inlined directives allocas should be emitted to entry block of the inner most 
containing outlined function"; ` 
or something less verbose.

Inlined directives should emit their `alloca`s to the entry block of the 
containing outined function, right? For that reason, and since we don't store 
an `alloca` insertion point in the OMPBuilder, I pass an empty insertion point, 
and back up the current point, without changing it.
I was planning, in the future, to keep a copy of the most recent alloca 
insertion point which we set when we outline, and pass that for `BodyCB`s 
within inlined directives.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3060
+? nullptr
+: Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.Int32Ty, false);
+

jdoerfert wrote:
> Can we do
> ```
> llvm::Value *HintInst = nullptr;
> if (Hint)
>   ...
> ```
> these three lines look ugly :(
np.

btw, the todo note is about the `CGM.Int32Ty`. Currently in clang it's 
`CGM.IntTy`. I'll expand on the todo



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3076
+  EmitBranchThroughCleanup(Dest);
+};
+

jdoerfert wrote:
> I somehow have the feeling we could have a "FiniCB" helper function as they 
> seem to be always the same. A TODO mentioning that is also OK for now.
I'll do a Todo for now. I am using a very similar `FiniCB` to the one you used 
for `parallel`. So it doesn't make sense to change these here and not that one 
with them.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3099
+  ReturnBlock = OldReturnBlock;
+};
+

jdoerfert wrote:
> Same here, that looks like the other "BodyGenCB". We should really make them 
> helpers. 
Same as prev.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:68
+  // Type::getInt32Ty(M.getContext()),/*NumElements*/ 8);
+  //  KmpCriticalNamePtrTy = PointerType::getUnqual(KmpCriticalNameTy);
   // Create all simple and struct types exposed by the runtime and remember

jdoerfert wrote:
> These comments that reference `KmpCriticalNameTy` do not help here. If you 
> want, feel free to add comments that explain how the result looks in a 
> generic way but only showing `KmpCriticalNameTy` seems counterproductive to 
> me.
I was testing something, and I forgot to remove them when I was done.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:717
+  // emit exit call and do any needed finalization.
+  auto FinIP = InsertPointTy(FiniBB, FiniBB->getFirstInsertionPt());
+  assert(FiniBB->getTerminator()->getNumSuccessors() == 1 &&

jdoerfert wrote:
> Nit: `InsertPointTy FiniIP(FiniBB, FiniBB->getFirstInsertionPt());`
Nit?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:721
+ "Unexpected insertion point for finalization call!");
+  emitCommonDirectiveExit(OMPD, FinIP, ExitBB, ExitCall, /*hasFinalize*/ true);
+

jdoerfert wrote:
> Shouldn't we use the argument here?
"The argument" refers to?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:808
+ StringRef FirstSeparator,
+ StringRef Separator) const {
+  SmallString<128> Buffer;

jdoerfert wrote:
> This can be a static helper and the name should be more descriptive, 
> `getName` is very generic.
This is taken from clang (I think CG_OMP)  as is. I am keeping everything as is 
for now in case somebody decided to help and came across it being used 
somewhere else, they should recognize it immediately.

So for now, I suggest a todo. cos if I find that it's not used except with 
`critical`, I'll merge all 3 functions into 1 and be done with it.



Comment at: 

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

One more mis-constification that I can't explain, nor reduce to a small test 
case (when I extract the code, the check behaves as expected / desired)

  namespace util {
struct StringIgnoreInitialHash : public std::unary_function
{
   size_t operator()(const std::string& rhs) const
   {
  std::size_t hash = 0;
  std::string::const_iterator str = rhs.begin();
  std::string::const_iterator end = rhs.end();
  if (str != end)
  {
 boost::hash_combine(hash, std::toupper(*str, std::locale()));
 ++str;
  }
  for (; str != end; ++str)
  {
 boost::hash_combine(hash, *str);
  }
  return hash;
   }
};
  }

This is in a header included 29 times in other headers and 106 times directly 
in translation units.

The const-checker reported 31 times that the variable 'hash' can be made 
constant.

Also it reported 5 times that the variable 'str' can be made constant (which it 
can't) and 194 times that variable 'end' can be made constant (which it can, 
and should).

Running in fix mode, made 'hash', 'str' and 'end' all constants.

Extracting this into its own translation unit and adding the requisite 
\#includes and paths to the boost files does not reproduce the error. Only 
'end' is, correctly, reported as needing to be const.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Summary of "misses"

One:

  uint32_t counter = 0;
  BOOST_FOREACH(const Foo* foo, *theFoos)
  {
  if (foo->getType() == someType)
  {
  ++counter;
  }
  }

The -fix makes the counter const :), so obviously it breaks compilation.

Two:

It marks a lot of std::stringstream / std::ostringstream instances as const. So 
we can't << into it, breaking compilation.

Three:

It marked an array in the header as const. There were some pointer variables 
(in a translation unit which included that header) that were declared and 
initialized to 0 (meaning nullptr), then later in a case block of a switch they 
were assigned [0] value. It did not change the pointer variables, but 
they used to be mutable pointer to mutable, so now the assignment from a const 
array failed to build.

The changes required to get the build complete (7500 translation units);

  15 files changed, 59 insertions(+), 59 deletions(-)

I can live with the breakage in the 3rd case - since the fix is one time and 
straightforward (and makes the code clearly better). But I prefer not to have 
to annotate all std::sotringstream instances with NOLINT. Also we have a fair 
number of BOOST_FOREACH macros from way back then. They should be ripped out of 
course, but it's in code that wasn't touched in a while, so having the 
clang-tidy keep reporting / misfixing that area is bothersome.

I really want this clang-tidy tool, but I don't want to put off users via false 
positives. Many don't want to deal with it anyway, and the smallest excuse will 
be brought up against enabling the use of the tool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D72500: [clangd] Show hower info for expressions

[liu hui via Phabricator via cfe-commits]

lh123 added a comment.

In D72500#1813975 , @sammccall wrote:

> I think we should avoid triggering for literals. Maybe some exceptions, but a 
> hover saying that 0 is an int with value 0 seems silly.


Yes, hovering over `IntegerLiteral/FloatingLiteral` may be useless, but I think 
it's useful when hovering over `StringLiteral/UserDefinedLiteral`.

+ hovering over "hello" show `char [6]`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72500



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

I have built diff14 and tried to apply it on an internal code base with ~7500 
translation units.

  $ python3 /opt/clang10/share/clang/run-clang-tidy.py -clang-tidy-binary  
/opt/clang10/bin/clang-tidy -clang-apply-replacements 
/opt/clang10/bin/clang-apply-replacements -p ../build.clang10.dbg/ 
"-checks=-*,cppcoreguidelines-const-correctness"  -fix -j 24 -quiet 
-deduplicate $(find lib -name \*.cpp) > const_fix.log 2&> const_err.log

The diffstat is

  1608 files changed, 19927 insertions(+), 19927 deletions(-)

This generated 56 "const const" declarations, of which 25 were triple const!

I have tried -deduplicate argument as in Jonas' script, but ...

  run-clang-tidy.py: error: unrecognized arguments: -deduplicate

Now onto editing those files by hand to remove the duplications and re-running 
the build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D71823: Support Swift calling convention for WebAssembly targets

[Yuta Saito via Phabricator via cfe-commits]

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



Comment at: clang/lib/Basic/Targets/WebAssembly.h:122
+case CC_Swift:
+  return CCCR_OK;
+default:

sbc100 wrote:
> This seems to disagree with the longer list in `callingConvSupported`.Is 
> this going to generate unnecessary warnings?
By default, `checkCallingConvention` allows only `CC_C` due to it's super class 
implementation. This change just allow to use SwiftCC and there is no effect to 
other CC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71823



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

[PATCH] D72239: [clang-tidy] new opencl recursion not supported check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a comment.

In D72239#1815583 , @JonasToth wrote:

> The other recursion check seems more sophisticated. What is your take on it? 
> Would you consider it better as well, what is your opinion on it?


I agree, I would go with the other check if I had to choose between the two.  
Since @lebedev.ri's version uses an existing CallGraph structure instead of 
re-creating it (like we did), I'd guess it is more performant than our version. 
Plus it doesn't have a user-defined recursion depth limit, which is one less 
thing for the user to worry about.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D72239



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

[PATCH] D70568: [Support] Possibly use exception handler in the Crash Recovery Context in the same way as global exceptions

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa1f16998f371: [Support] Optionally call signal handlers when 
a function wrapped by the the… (authored by aganea).

Changed prior to commit:
  https://reviews.llvm.org/D70568?vs=237038=237518#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70568

Files:
  llvm/include/llvm/Support/CrashRecoveryContext.h
  llvm/include/llvm/Support/Signals.h
  llvm/lib/Support/CrashRecoveryContext.cpp
  llvm/lib/Support/Unix/Signals.inc
  llvm/lib/Support/Windows/Signals.inc
  llvm/unittests/Support/CrashRecoveryTest.cpp

Index: llvm/unittests/Support/CrashRecoveryTest.cpp
===
--- llvm/unittests/Support/CrashRecoveryTest.cpp
+++ llvm/unittests/Support/CrashRecoveryTest.cpp
@@ -8,6 +8,8 @@
 
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Signals.h"
 #include "gtest/gtest.h"
 
 #ifdef _WIN32
@@ -23,6 +25,7 @@
 static void nullDeref() { *(volatile int *)0x10 = 0; }
 static void incrementGlobal() { ++GlobalInt; }
 static void llvmTrap() { LLVM_BUILTIN_TRAP; }
+static void incrementGlobalWithParam(void *) { ++GlobalInt; }
 
 TEST(CrashRecoveryTest, Basic) {
   llvm::CrashRecoveryContext::Enable();
@@ -58,6 +61,33 @@
 EXPECT_FALSE(CRC.RunSafely(nullDeref));
   } // run cleanups
   EXPECT_EQ(1, GlobalInt);
+  llvm::CrashRecoveryContext::Disable();
+}
+
+TEST(CrashRecoveryTest, DumpStackCleanup) {
+  SmallString<128> Filename;
+  std::error_code EC = sys::fs::createTemporaryFile("crash", "test", Filename);
+  EXPECT_FALSE(EC);
+  sys::RemoveFileOnSignal(Filename);
+  llvm::sys::AddSignalHandler(incrementGlobalWithParam, nullptr);
+  GlobalInt = 0;
+  llvm::CrashRecoveryContext::Enable();
+  {
+CrashRecoveryContext CRC;
+CRC.DumpStackAndCleanupOnFailure = true;
+EXPECT_TRUE(CRC.RunSafely(noop));
+  }
+  EXPECT_TRUE(sys::fs::exists(Filename));
+  EXPECT_EQ(GlobalInt, 0);
+  {
+CrashRecoveryContext CRC;
+CRC.DumpStackAndCleanupOnFailure = true;
+EXPECT_FALSE(CRC.RunSafely(nullDeref));
+EXPECT_NE(CRC.RetCode, 0);
+  }
+  EXPECT_FALSE(sys::fs::exists(Filename));
+  EXPECT_EQ(GlobalInt, 1);
+  llvm::CrashRecoveryContext::Disable();
 }
 
 #ifdef _WIN32
Index: llvm/lib/Support/Windows/Signals.inc
===
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -521,10 +521,13 @@
 extern "C" VOID WINAPI RtlCaptureContext(PCONTEXT ContextRecord);
 #endif
 
-void llvm::sys::PrintStackTrace(raw_ostream ) {
-  STACKFRAME64 StackFrame = {};
-  CONTEXT Context = {};
-  ::RtlCaptureContext();
+static void LocalPrintStackTrace(raw_ostream , PCONTEXT C) {
+  STACKFRAME64 StackFrame{};
+  CONTEXT Context{};
+  if (!C) {
+::RtlCaptureContext();
+C = 
+  }
 #if defined(_M_X64)
   StackFrame.AddrPC.Offset = Context.Rip;
   StackFrame.AddrStack.Offset = Context.Rsp;
@@ -546,9 +549,12 @@
   StackFrame.AddrStack.Mode = AddrModeFlat;
   StackFrame.AddrFrame.Mode = AddrModeFlat;
   PrintStackTraceForThread(OS, GetCurrentProcess(), GetCurrentThread(),
-   StackFrame, );
+   StackFrame, C);
 }
 
+void llvm::sys::PrintStackTrace(raw_ostream ) {
+  LocalPrintStackTrace(OS, nullptr);
+}
 
 void llvm::sys::SetInterruptFunction(void (*IF)()) {
   RegisterHandler();
@@ -792,6 +798,10 @@
   return std::error_code();
 }
 
+void sys::CleanupOnSignal(uintptr_t Context) {
+  LLVMUnhandledExceptionFilter((LPEXCEPTION_POINTERS)Context);
+}
+
 static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
@@ -810,42 +820,9 @@
<< "\n";
   }
 
-  // Initialize the STACKFRAME structure.
-  STACKFRAME64 StackFrame = {};
-
-#if defined(_M_X64)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Rbp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_IX86)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Eip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Esp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Ebp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_ARM64) || defined(_M_ARM)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Pc;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Sp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-#if defined(_M_ARM64)
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Fp;
-#else
-  StackFrame.AddrFrame.Offset = 

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61775 tests passed, 0 failed 
and 780 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553



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

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 237517.
njames93 added a comment.

- Rebased onto trunk fixing merge issues


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-pre-increment.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-prefer-pre-increment.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment-disable-cpp-opcalls.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+template 
+class Iterator {
+  T *Current;
+
+public:
+  Iterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  Iterator ++() { return ++Current, *this; }
+  Iterator operator++(int) {
+Iterator Copy = *this;
+++Current;
+return Copy;
+  }
+  Iterator () { return --Current, *this; }
+  Iterator operator--(int) {
+Iterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+template 
+class PostfixIterator {
+  T *Current;
+
+public:
+  PostfixIterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  PostfixIterator operator++(int) {
+PostfixIterator Copy = *this;
+++Current;
+return Copy;
+  }
+  PostfixIterator operator--(int) {
+PostfixIterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+void foo() {
+  int Array[32];
+  Iterator It([0]);
+  It++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++It;
+  It--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --It;
+  (*It)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++(*It);
+  (*It)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --(*It);
+
+  *It++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  *It--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+
+  PostfixIterator PfIt([0]);
+  PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES-NOT: {{^}}  ++PfIt;
+  PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES-NOT: {{^}}  --PfIt;
+  (*PfIt)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++(*PfIt);
+  (*PfIt)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --(*PfIt);
+
+  *PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  *PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+}
Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+#define INC(X) X++
+#define DEC(X) X--
+
+void foo(int A) {
+  for (int I = 0; I < 10; I++) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-increment instead of Post-increment
+// CHECK-FIXES: {{^}}  for (int I = 0; I < 10; ++I) {
+  }
+  for (int I = 0; I < 10; ++I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  }
+  for (int I = 0; I < 10; A = I++) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  }
+
+  for (int I = 10; I < 0; I--) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-decrement instead of Post-decrement
+// CHECK-FIXES: {{^}}  for (int I = 10; I < 0; --I) {
+  }
+  for (int I = 10; I < 0; --I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of 

[PATCH] D72560: Add -Wrange-loop-analysis changes to ReleaseNotes

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 2 inline comments as done.
Mordante added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72560



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

[clang] dc422e9 - Add -Wrange-loop-analysis changes to ReleaseNotes

[Mark de Wever via cfe-commits]

Author: Mark de Wever
Date: 2020-01-11T19:56:34+01:00
New Revision: dc422e968e73790178e500f506e8fb7cfa1e62ea

URL: 
https://github.com/llvm/llvm-project/commit/dc422e968e73790178e500f506e8fb7cfa1e62ea
DIFF: 
https://github.com/llvm/llvm-project/commit/dc422e968e73790178e500f506e8fb7cfa1e62ea.diff

LOG: Add -Wrange-loop-analysis changes to ReleaseNotes

This reflects the recent changes done.

Added: 


Modified: 
clang/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index eee63c8e9239..e131e4f12780 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -64,6 +64,10 @@ Improvements to Clang's diagnostics
 - -Wbitwise-conditional-parentheses will warn on operator precedence issues
   when mixing bitwise-and (&) and bitwise-or (|) operator with the
   conditional operator (?:).
+- -Wrange-loop-analysis got several improvements. It no longer warns about a
+  copy being made when the result is bound to an rvalue reference. It no longer
+  warns when an object of a small, trivially copyable type is copied. The
+  warning now offers fixits. It is now part of -Wall.
 
 Non-comprehensive list of changes in this release
 -



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

[PATCH] D72560: Add -Wrange-loop-analysis changes to ReleaseNotes

[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 with some suggestions.




Comment at: clang/docs/ReleaseNotes.rst:69
+  copy being made when the result is bound to an rvalue reference. It no longer
+  warns when an object of a small trivially copyable type is copied. The
+  warning now offers fixits.

small trivially copyable -> small, trivially copyable



Comment at: clang/docs/ReleaseNotes.rst:71
+  warning now offers fixits.
+- -Wrange-loop-analysis is now part of -Wall.
 

I'd fold this in to the previous bullet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72560



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

[clang] 23a799a - Revert "[ASTMatchers] extract public matchers from const-analysis into own patch"

[Jonas Toth via cfe-commits]

Author: Jonas Toth
Date: 2020-01-11T19:41:27+01:00
New Revision: 23a799adf0abbe9a7be1494d5efd1ab3215ee4fb

URL: 
https://github.com/llvm/llvm-project/commit/23a799adf0abbe9a7be1494d5efd1ab3215ee4fb
DIFF: 
https://github.com/llvm/llvm-project/commit/23a799adf0abbe9a7be1494d5efd1ab3215ee4fb.diff

LOG: Revert "[ASTMatchers] extract public matchers from const-analysis into own 
patch"

This reverts commit 4c48ea68e491cb42f1b5d43ffba89f6a7f0dadc4.
The powerpc buildbots had an internal compiler error after this patch.
This requires some inspection.

Added: 


Modified: 
clang/docs/LibASTMatchersReference.html
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/lib/ASTMatchers/Dynamic/Registry.cpp
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Removed: 




diff  --git a/clang/docs/LibASTMatchersReference.html 
b/clang/docs/LibASTMatchersReference.html
index 099462f17d2b..5bb181b04d3a 100644
--- a/clang/docs/LibASTMatchersReference.html
+++ b/clang/docs/LibASTMatchersReference.html
@@ -635,30 +635,6 @@ Node Matchers
 
 
 
-Matcherhttps://clang.llvm.org/doxygen/classclang_1_1DecompositionDecl.html;>DecompositionDecldecompositionDeclMatcherhttps://clang.llvm.org/doxygen/classclang_1_1DecompositionDecl.html;>DecompositionDecl...
-Matches 
decomposition-declarations.
-
-Examples matches the declaration node with foo and bar, but not
-number.
-(matcher = declStmt(has(decompositionDecl(
-
-  int number = 42;
-  auto [foo, bar] = std::make_pair{42, 42};
-
-
-
-Matcherhttps://clang.llvm.org/doxygen/classclang_1_1DecompositionDecl.html;>DecompositionDecldecompositionDeclMatcherhttps://clang.llvm.org/doxygen/classclang_1_1DecompositionDecl.html;>DecompositionDecl...
-Matches 
decomposition-declarations.
-
-Examples matches the declaration node with foo and bar, but not
-number.
-(matcher = declStmt(has(decompositionDecl(
-
-  int number = 42;
-  auto [foo, bar] = std::make_pair{42, 42};
-
-
-
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1NestedNameSpecifierLoc.html;>NestedNameSpecifierLocnestedNameSpecifierLocMatcherhttps://clang.llvm.org/doxygen/classclang_1_1NestedNameSpecifierLoc.html;>NestedNameSpecifierLoc...
 Same as 
nestedNameSpecifier but matches NestedNameSpecifierLoc.
 
@@ -4993,60 +4969,6 @@ AST Traversal Matchers
 
 
 
-Matcherhttps://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html;>CXXConstructExprforEachArgumentWithParamTypeMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
ArgMatcher, Matcherhttps://clang.llvm.org/doxygen/classclang_1_1QualType.html;>QualType
 ParamMatcher
-Matches all arguments and their 
respective types for a CallExpr or
-CXXConstructExpr. It is very similar to forEachArgumentWithParam but
-it works on calls through function pointers as well.
-
-The 
diff erence is, that function pointers do not provide access to a
-ParmVarDecl, but only the QualType for each argument.
-
-Given
-  void f(int i);
-  int y;
-  f(y);
-  void (*f_ptr)(int) = f;
-  f_ptr(y);
-callExpr(
-  forEachArgumentWithParamType(
-declRefExpr(to(varDecl(hasName("y",
-qualType(isInteger()).bind("type)
-))
-  matches f(y) and f_ptr(y)
-with declRefExpr(...)
-  matching int y
-and qualType(...)
-  matching int
-
-
-
-Matcherhttps://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html;>CXXConstructExprforEachArgumentWithParamTypeMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
ArgMatcher, Matcherhttps://clang.llvm.org/doxygen/classclang_1_1QualType.html;>QualType
 ParamMatcher
-Matches all arguments and their 
respective types for a CallExpr or
-CXXConstructExpr. It is very similar to forEachArgumentWithParam but
-it works on calls through function pointers as well.
-
-The 
diff erence is, that function pointers do not provide access to a
-ParmVarDecl, but only the QualType for each argument.
-
-Given
-  void f(int i);
-  int y;
-  f(y);
-  void (*f_ptr)(int) = f;
-  f_ptr(y);
-callExpr(
-  forEachArgumentWithParamType(
-declRefExpr(to(varDecl(hasName("y",
-qualType(isInteger()).bind("type)
-))
-  matches f(y) and f_ptr(y)
-with declRefExpr(...)
-  matching int y
-and qualType(...)
-  matching int
-
-
-
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html;>CXXConstructExprhasAnyArgumentMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
InnerMatcher
 Matches any argument 
of a call expression or a constructor call
 expression, or an ObjC-message-send expression.
@@ -5525,60 +5447,6 @@ AST Traversal Matchers
 
 
 
-Matcherhttps://clang.llvm.org/doxygen/classclang_1_1CallExpr.html;>CallExprforEachArgumentWithParamTypeMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
ArgMatcher, Matcherhttps://clang.llvm.org/doxygen/classclang_1_1QualType.html;>QualType
 ParamMatcher
-Matches all arguments and their 
respective types for a CallExpr or
-CXXConstructExpr. It is very 

[PATCH] D72560: Add -Wrange-loop-analysis changes to ReleaseNotes

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61772 tests passed, 0 failed 
and 780 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72560



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

[PATCH] D72505: [ASTMatchers] extract public matchers from const-analysis into own patch

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4c48ea68e491: [ASTMatchers] extract public matchers from 
const-analysis into own patch (authored by JonasToth).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72505

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -735,6 +735,172 @@
 std::make_unique>("v", 4)));
 }
 
+TEST(ForEachArgumentWithParamType, ReportsNoFalsePositives) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  // IntParam does not match.
+  EXPECT_TRUE(notMatches("void f(int* i) { int* y; f(y); }", CallExpr));
+  // ArgumentY does not match.
+  EXPECT_TRUE(notMatches("void f(int i) { int x; f(x); }", CallExpr));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesCXXMemberCallExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct S {"
+  "  const S& operator[](int i) { return *this; }"
+  "};"
+  "void f(S S1) {"
+  "  int y = 1;"
+  "  S1[y];"
+  "}",
+CallExpr, std::make_unique>("type", 1)));
+
+  StatementMatcher CallExpr2 =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct S {"
+  "  static void g(int i);"
+  "};"
+  "void f() {"
+  "  int y = 1;"
+  "  S::g(y);"
+  "}",
+CallExpr2, std::make_unique>("type", 1)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesCallExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(
+matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+ std::make_unique>(
+   "type")));
+  EXPECT_TRUE(
+matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+ std::make_unique>(
+   "arg")));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void f(int i, int j) { int y; f(y, y); }", CallExpr,
+std::make_unique>("type", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void f(int i, int j) { int y; f(y, y); }", CallExpr,
+std::make_unique>("arg", 2)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesConstructExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher ConstructExpr =
+cxxConstructExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct C {"
+  "  C(int i) {}"
+  "};"
+  "int y = 0;"
+  "C Obj(y);",
+ConstructExpr,
+std::make_unique>("type")));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct C {"
+  "  C(int i) {}"
+  "};"
+  "int y = 0;"
+  "C Obj(y);",
+ConstructExpr,
+std::make_unique>("arg")));
+}
+
+TEST(ForEachArgumentWithParamType, HandlesKandRFunctions) {
+  StatementMatcher ArgumentY =
+  declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+  callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(matchesC("void f();\n"
+   "void call_it(void) { int x, y; f(x, y); }\n"
+   "void f(a, b) int a, b; {}\n"
+   "void call_it2(void) { int x, y; f(x, y); }",
+   CallExpr));
+}
+
+TEST(ForEachArgumentWithParamType, HandlesBoundNodesForNonMatches) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void g(int i, int j) {"
+  "  int a;"
+  "  int b;"
+  "  int c;"
+  "  g(a, 0);"
+  "  g(a, b);"
+  "  g(0, b);"
+  "}",
+functionDecl(
+  forEachDescendant(varDecl().bind("v")),
+  forEachDescendant(callExpr(forEachArgumentWithParamType(
+declRefExpr(to(decl(equalsBoundNode("v", qualType(),
+

[clang] 4c48ea6 - [ASTMatchers] extract public matchers from const-analysis into own patch

[Jonas Toth via cfe-commits]

Author: Jonas Toth
Date: 2020-01-11T19:21:03+01:00
New Revision: 4c48ea68e491cb42f1b5d43ffba89f6a7f0dadc4

URL: 
https://github.com/llvm/llvm-project/commit/4c48ea68e491cb42f1b5d43ffba89f6a7f0dadc4
DIFF: 
https://github.com/llvm/llvm-project/commit/4c48ea68e491cb42f1b5d43ffba89f6a7f0dadc4.diff

LOG: [ASTMatchers] extract public matchers from const-analysis into own patch

Summary:
The analysis for const-ness of local variables required a view generally useful
matchers that are extracted into its own patch.

They are `decompositionDecl` and `forEachArgumentWithParamType`, that works
for calls through function pointers as well.

Reviewers: aaron.ballman

Reviewed By: aaron.ballman

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D72505

Added: 


Modified: 
clang/docs/LibASTMatchersReference.html
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/lib/ASTMatchers/Dynamic/Registry.cpp
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Removed: 




diff  --git a/clang/docs/LibASTMatchersReference.html 
b/clang/docs/LibASTMatchersReference.html
index 5bb181b04d3a..099462f17d2b 100644
--- a/clang/docs/LibASTMatchersReference.html
+++ b/clang/docs/LibASTMatchersReference.html
@@ -635,6 +635,30 @@ Node Matchers
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1DecompositionDecl.html;>DecompositionDecldecompositionDeclMatcherhttps://clang.llvm.org/doxygen/classclang_1_1DecompositionDecl.html;>DecompositionDecl...
+Matches 
decomposition-declarations.
+
+Examples matches the declaration node with foo and bar, but not
+number.
+(matcher = declStmt(has(decompositionDecl(
+
+  int number = 42;
+  auto [foo, bar] = std::make_pair{42, 42};
+
+
+
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1DecompositionDecl.html;>DecompositionDecldecompositionDeclMatcherhttps://clang.llvm.org/doxygen/classclang_1_1DecompositionDecl.html;>DecompositionDecl...
+Matches 
decomposition-declarations.
+
+Examples matches the declaration node with foo and bar, but not
+number.
+(matcher = declStmt(has(decompositionDecl(
+
+  int number = 42;
+  auto [foo, bar] = std::make_pair{42, 42};
+
+
+
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1NestedNameSpecifierLoc.html;>NestedNameSpecifierLocnestedNameSpecifierLocMatcherhttps://clang.llvm.org/doxygen/classclang_1_1NestedNameSpecifierLoc.html;>NestedNameSpecifierLoc...
 Same as 
nestedNameSpecifier but matches NestedNameSpecifierLoc.
 
@@ -4969,6 +4993,60 @@ AST Traversal Matchers
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html;>CXXConstructExprforEachArgumentWithParamTypeMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
ArgMatcher, Matcherhttps://clang.llvm.org/doxygen/classclang_1_1QualType.html;>QualType
 ParamMatcher
+Matches all arguments and their 
respective types for a CallExpr or
+CXXConstructExpr. It is very similar to forEachArgumentWithParam but
+it works on calls through function pointers as well.
+
+The 
diff erence is, that function pointers do not provide access to a
+ParmVarDecl, but only the QualType for each argument.
+
+Given
+  void f(int i);
+  int y;
+  f(y);
+  void (*f_ptr)(int) = f;
+  f_ptr(y);
+callExpr(
+  forEachArgumentWithParamType(
+declRefExpr(to(varDecl(hasName("y",
+qualType(isInteger()).bind("type)
+))
+  matches f(y) and f_ptr(y)
+with declRefExpr(...)
+  matching int y
+and qualType(...)
+  matching int
+
+
+
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html;>CXXConstructExprforEachArgumentWithParamTypeMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
ArgMatcher, Matcherhttps://clang.llvm.org/doxygen/classclang_1_1QualType.html;>QualType
 ParamMatcher
+Matches all arguments and their 
respective types for a CallExpr or
+CXXConstructExpr. It is very similar to forEachArgumentWithParam but
+it works on calls through function pointers as well.
+
+The 
diff erence is, that function pointers do not provide access to a
+ParmVarDecl, but only the QualType for each argument.
+
+Given
+  void f(int i);
+  int y;
+  f(y);
+  void (*f_ptr)(int) = f;
+  f_ptr(y);
+callExpr(
+  forEachArgumentWithParamType(
+declRefExpr(to(varDecl(hasName("y",
+qualType(isInteger()).bind("type)
+))
+  matches f(y) and f_ptr(y)
+with declRefExpr(...)
+  matching int y
+and qualType(...)
+  matching int
+
+
+
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html;>CXXConstructExprhasAnyArgumentMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
InnerMatcher
 Matches any argument 
of a call expression or a constructor call
 expression, or an ObjC-message-send expression.
@@ -5447,6 +5525,60 @@ AST Traversal Matchers
 
 
 

[PATCH] D72560: Add -Wrange-loop-analysis changes to ReleaseNotes

[Mark de Wever via Phabricator via cfe-commits]

Mordante created this revision.
Mordante added reviewers: aaron.ballman, xbolva00.
Mordante added a project: clang.

This reflects the recent changes done.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72560

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -64,6 +64,11 @@
 - -Wbitwise-conditional-parentheses will warn on operator precedence issues
   when mixing bitwise-and (&) and bitwise-or (|) operator with the
   conditional operator (?:).
+- -Wrange-loop-analysis got several improvements. It no longer warns about a
+  copy being made when the result is bound to an rvalue reference. It no longer
+  warns when an object of a small trivially copyable type is copied. The
+  warning now offers fixits.
+- -Wrange-loop-analysis is now part of -Wall.
 
 Non-comprehensive list of changes in this release
 -


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -64,6 +64,11 @@
 - -Wbitwise-conditional-parentheses will warn on operator precedence issues
   when mixing bitwise-and (&) and bitwise-or (|) operator with the
   conditional operator (?:).
+- -Wrange-loop-analysis got several improvements. It no longer warns about a
+  copy being made when the result is bound to an rvalue reference. It no longer
+  warns when an object of a small trivially copyable type is copied. The
+  warning now offers fixits.
+- -Wrange-loop-analysis is now part of -Wall.
 
 Non-comprehensive list of changes in this release
 -
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72505: [ASTMatchers] extract public matchers from const-analysis into own patch

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 237514.
JonasToth added a comment.

- remove merge conflict markers in docs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72505

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -735,6 +735,172 @@
 std::make_unique>("v", 4)));
 }
 
+TEST(ForEachArgumentWithParamType, ReportsNoFalsePositives) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  // IntParam does not match.
+  EXPECT_TRUE(notMatches("void f(int* i) { int* y; f(y); }", CallExpr));
+  // ArgumentY does not match.
+  EXPECT_TRUE(notMatches("void f(int i) { int x; f(x); }", CallExpr));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesCXXMemberCallExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct S {"
+  "  const S& operator[](int i) { return *this; }"
+  "};"
+  "void f(S S1) {"
+  "  int y = 1;"
+  "  S1[y];"
+  "}",
+CallExpr, std::make_unique>("type", 1)));
+
+  StatementMatcher CallExpr2 =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct S {"
+  "  static void g(int i);"
+  "};"
+  "void f() {"
+  "  int y = 1;"
+  "  S::g(y);"
+  "}",
+CallExpr2, std::make_unique>("type", 1)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesCallExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(
+matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+ std::make_unique>(
+   "type")));
+  EXPECT_TRUE(
+matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+ std::make_unique>(
+   "arg")));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void f(int i, int j) { int y; f(y, y); }", CallExpr,
+std::make_unique>("type", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void f(int i, int j) { int y; f(y, y); }", CallExpr,
+std::make_unique>("arg", 2)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesConstructExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher ConstructExpr =
+cxxConstructExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct C {"
+  "  C(int i) {}"
+  "};"
+  "int y = 0;"
+  "C Obj(y);",
+ConstructExpr,
+std::make_unique>("type")));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct C {"
+  "  C(int i) {}"
+  "};"
+  "int y = 0;"
+  "C Obj(y);",
+ConstructExpr,
+std::make_unique>("arg")));
+}
+
+TEST(ForEachArgumentWithParamType, HandlesKandRFunctions) {
+  StatementMatcher ArgumentY =
+  declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+  callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(matchesC("void f();\n"
+   "void call_it(void) { int x, y; f(x, y); }\n"
+   "void f(a, b) int a, b; {}\n"
+   "void call_it2(void) { int x, y; f(x, y); }",
+   CallExpr));
+}
+
+TEST(ForEachArgumentWithParamType, HandlesBoundNodesForNonMatches) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void g(int i, int j) {"
+  "  int a;"
+  "  int b;"
+  "  int c;"
+  "  g(a, 0);"
+  "  g(a, b);"
+  "  g(0, b);"
+  "}",
+functionDecl(
+  forEachDescendant(varDecl().bind("v")),
+  forEachDescendant(callExpr(forEachArgumentWithParamType(
+declRefExpr(to(decl(equalsBoundNode("v", qualType(),
+std::make_unique>("v", 4)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesFunctionPtrCalls) {
+  

[PATCH] D72505: [ASTMatchers] extract public matchers from const-analysis into own patch

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 237511.
JonasToth added a comment.

- fix compilation error, missing getType


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72505

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -735,6 +735,172 @@
 std::make_unique>("v", 4)));
 }
 
+TEST(ForEachArgumentWithParamType, ReportsNoFalsePositives) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  // IntParam does not match.
+  EXPECT_TRUE(notMatches("void f(int* i) { int* y; f(y); }", CallExpr));
+  // ArgumentY does not match.
+  EXPECT_TRUE(notMatches("void f(int i) { int x; f(x); }", CallExpr));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesCXXMemberCallExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct S {"
+  "  const S& operator[](int i) { return *this; }"
+  "};"
+  "void f(S S1) {"
+  "  int y = 1;"
+  "  S1[y];"
+  "}",
+CallExpr, std::make_unique>("type", 1)));
+
+  StatementMatcher CallExpr2 =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct S {"
+  "  static void g(int i);"
+  "};"
+  "void f() {"
+  "  int y = 1;"
+  "  S::g(y);"
+  "}",
+CallExpr2, std::make_unique>("type", 1)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesCallExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(
+matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+ std::make_unique>(
+   "type")));
+  EXPECT_TRUE(
+matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+ std::make_unique>(
+   "arg")));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void f(int i, int j) { int y; f(y, y); }", CallExpr,
+std::make_unique>("type", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void f(int i, int j) { int y; f(y, y); }", CallExpr,
+std::make_unique>("arg", 2)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesConstructExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher ConstructExpr =
+cxxConstructExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct C {"
+  "  C(int i) {}"
+  "};"
+  "int y = 0;"
+  "C Obj(y);",
+ConstructExpr,
+std::make_unique>("type")));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct C {"
+  "  C(int i) {}"
+  "};"
+  "int y = 0;"
+  "C Obj(y);",
+ConstructExpr,
+std::make_unique>("arg")));
+}
+
+TEST(ForEachArgumentWithParamType, HandlesKandRFunctions) {
+  StatementMatcher ArgumentY =
+  declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+  callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(matchesC("void f();\n"
+   "void call_it(void) { int x, y; f(x, y); }\n"
+   "void f(a, b) int a, b; {}\n"
+   "void call_it2(void) { int x, y; f(x, y); }",
+   CallExpr));
+}
+
+TEST(ForEachArgumentWithParamType, HandlesBoundNodesForNonMatches) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void g(int i, int j) {"
+  "  int a;"
+  "  int b;"
+  "  int c;"
+  "  g(a, 0);"
+  "  g(a, b);"
+  "  g(0, b);"
+  "}",
+functionDecl(
+  forEachDescendant(varDecl().bind("v")),
+  forEachDescendant(callExpr(forEachArgumentWithParamType(
+declRefExpr(to(decl(equalsBoundNode("v", qualType(),
+std::make_unique>("v", 4)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesFunctionPtrCalls) {
+  

[PATCH] D72362: [clang-tidy] misc-no-recursion: a new check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 8 inline comments as done.
lebedev.ri added a comment.

Thanks for taking a look.
Some deflective replies inline.




Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:21
+
+inline bool operator==(const CallGraphNode::CallRecord ,
+   const CallGraphNode::CallRecord ) {

JonasToth wrote:
> That should be in the `CallGraph` code, adding a private operator overload 
> does not feel right.
I didn't want to put this into callgraph, because this
cements the fact that we do not care about sourceloc,
which may not be true for other users.
I can put it there, but if told so by it's code owners (@NoQ ?)



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:31
+
+// Specialize DenseMapInfo for clang::CallGraphNode::CallRecord.
+template <> struct DenseMapInfo {

JonasToth wrote:
> That stuff, too.
(same reasoning)



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:67
+// 2. it is immutable, the way it was constructed it will stay.
+template  class ImmutableSmartSet {
+  ArrayRef Vector;

JonasToth wrote:
> Why `smart`?
Because if it's just named `ImmutableSet`, why should it not just be a `using 
ImmutableSet = DenseSet; const ImmutableSet ..`?
This denotes it's different behaviour when in small-size mode and in 
non-small-size mode.



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:73
+
+  bool isSmall() const { return Set.empty(); }
+

JonasToth wrote:
> That method name looks confusing. `isEmpty()`? If not, why?
See how it's used, e.g. in `count()`, see `SmallSet` also.



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:242
+Decl *D = N->getDecl();
+diag(D->getLocation(), "function '%0' is part of call graph loop")
+<< cast(D)->getName();

JonasToth wrote:
> That should be a `Note:`
I'm intentionally emitting it as a warning.

If i `// NOLINT` the main warning, does that not silence the related `Notes`?
I don't want `NOLINT`ing the "main" function to silence the report for the rest 
of the functions in SCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72362



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

[PATCH] D72505: [ASTMatchers] extract public matchers from const-analysis into own patch

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 237510.
JonasToth marked 3 inline comments as done.
JonasToth added a comment.

- add test for K functions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72505

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -735,6 +735,172 @@
 std::make_unique>("v", 4)));
 }
 
+TEST(ForEachArgumentWithParamType, ReportsNoFalsePositives) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  // IntParam does not match.
+  EXPECT_TRUE(notMatches("void f(int* i) { int* y; f(y); }", CallExpr));
+  // ArgumentY does not match.
+  EXPECT_TRUE(notMatches("void f(int i) { int x; f(x); }", CallExpr));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesCXXMemberCallExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct S {"
+  "  const S& operator[](int i) { return *this; }"
+  "};"
+  "void f(S S1) {"
+  "  int y = 1;"
+  "  S1[y];"
+  "}",
+CallExpr, std::make_unique>("type", 1)));
+
+  StatementMatcher CallExpr2 =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct S {"
+  "  static void g(int i);"
+  "};"
+  "void f() {"
+  "  int y = 1;"
+  "  S::g(y);"
+  "}",
+CallExpr2, std::make_unique>("type", 1)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesCallExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(
+matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+ std::make_unique>(
+   "type")));
+  EXPECT_TRUE(
+matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+ std::make_unique>(
+   "arg")));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void f(int i, int j) { int y; f(y, y); }", CallExpr,
+std::make_unique>("type", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void f(int i, int j) { int y; f(y, y); }", CallExpr,
+std::make_unique>("arg", 2)));
+}
+
+TEST(ForEachArgumentWithParamType, MatchesConstructExpr) {
+  StatementMatcher ArgumentY =
+declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher ConstructExpr =
+cxxConstructExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct C {"
+  "  C(int i) {}"
+  "};"
+  "int y = 0;"
+  "C Obj(y);",
+ConstructExpr,
+std::make_unique>("type")));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"struct C {"
+  "  C(int i) {}"
+  "};"
+  "int y = 0;"
+  "C Obj(y);",
+ConstructExpr,
+std::make_unique>("arg")));
+}
+
+TEST(ForEachArgumentWithParamType, HandlesKandRFunctions) {
+  StatementMatcher ArgumentY =
+  declRefExpr(to(varDecl(hasName("y".bind("arg");
+  TypeMatcher IntType = qualType(isInteger()).bind("type");
+  StatementMatcher CallExpr =
+  callExpr(forEachArgumentWithParamType(ArgumentY, IntType));
+
+  EXPECT_TRUE(matchesC("void f();\n"
+   "void call_it(void) { int x, y; f(x, y); }\n"
+   "void f(a, b) int a, b; {}\n"
+   "void call_it2(void) { int x, y; f(x, y); }",
+   CallExpr));
+}
+
+TEST(ForEachArgumentWithParamType, HandlesBoundNodesForNonMatches) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+"void g(int i, int j) {"
+  "  int a;"
+  "  int b;"
+  "  int c;"
+  "  g(a, 0);"
+  "  g(a, b);"
+  "  g(0, b);"
+  "}",
+functionDecl(
+  forEachDescendant(varDecl().bind("v")),
+  forEachDescendant(callExpr(forEachArgumentWithParamType(
+declRefExpr(to(decl(equalsBoundNode("v", qualType(),
+std::make_unique>("v", 4)));
+}
+
+TEST(ForEachArgumentWithParamType, 

[PATCH] D71823: Support Swift calling convention for WebAssembly targets

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added inline comments.



Comment at: clang/lib/Basic/Targets/WebAssembly.h:122
+case CC_Swift:
+  return CCCR_OK;
+default:

This seems to disagree with the longer list in `callingConvSupported`.Is 
this going to generate unnecessary warnings?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71823



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Thank you for looking into this; I'll start building diff14 to test locally.

Thanks :)

> I'm good with merging this checker as-is, as long as it is not enabled by 
> default.

Which part shall be disabled in your opinion? The whole check? that would not 
work xD But I think both `value` and `reference` analysis is good enough. The 
issue with `auto&` is only a problem in templates, where different 
instantiations result to different modification-status if the type of the 
reference depends on the template instantiation.
Its not that common, but a false positive. :(

But i think it is already valueable in day-to-day programming and to modernize 
code, lets see what aaron says :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D72362: [clang-tidy] misc-no-recursion: a new check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

So that is were the CTU question comes from? :)




Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:21
+
+inline bool operator==(const CallGraphNode::CallRecord ,
+   const CallGraphNode::CallRecord ) {

That should be in the `CallGraph` code, adding a private operator overload does 
not feel right.



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:31
+
+// Specialize DenseMapInfo for clang::CallGraphNode::CallRecord.
+template <> struct DenseMapInfo {

That stuff, too.



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:67
+// 2. it is immutable, the way it was constructed it will stay.
+template  class ImmutableSmartSet {
+  ArrayRef Vector;

Why `smart`?



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:73
+
+  bool isSmall() const { return Set.empty(); }
+

That method name looks confusing. `isEmpty()`? If not, why?



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:242
+Decl *D = N->getDecl();
+diag(D->getLocation(), "function '%0' is part of call graph loop")
+<< cast(D)->getName();

That should be a `Note:`



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:287
+  diag(CyclicCallStack.back().Loc,
+   "... which was the starting point of this call graph",
+   DiagnosticIDs::Note);

Please merge these two notes into one.



Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.h:28
+class NoRecursionCheck : public ClangTidyCheck {
+  void handleSCC(ArrayRef SCC);
+

nit: the `private` stuff is usually at the bottom in the other clang-tidy code. 
not sure if there is something in the coding standard though.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:149
+
+  Finds strongly connected functions (by analyzing call graph for SCC's
+  that are loops), diagnoses each function in the cycle,

The technical details should not be in the release notes. Just that it finds 
recursion and diagnoses it.



Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp:127
+}
+
+// CHECK-NOTES: :[[@LINE-14]]:13: warning: function 
'indirect_recursion_with_depth2' is part of call graph loop [misc-no-recursion]

Does the check find recursion through function pointers? (Probably not? Should 
be noted as limitation).

Please add cases from lambdas. And cases where recursion happens in templates / 
only with one of multiple instantiations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72362



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1815568 , @JonasToth wrote:

> @0x8000- i did remove `dependentTypes` from matching. Locally that works 
> with your reported test-case (i was running clang-tidy wrong :().
>
> I do understand the `auto &` issues now (i think).
>  If the type is deduced to a template-type or something that depends the 
> standard ways of detecting that do not work, because in each template 
> instantiation this information is not available.
>
> Having a `auto & local = TemplatedVariable` does not have the 
> `isInstantiationDependentType()`-bits set and for each instantiation of the 
> template the const-analysis is run.
>  The false positives happened in different template instantiations, i think 
> because the overloaded operators were sometimes `const` and sometimes not. I 
> tried many ways to detect, if the type comes from a template, but had no 
> success.
>  This is probably an issue in `Sema` or requires adjustments there.
>  I added tests in clang-tidy, but `#if 0` them out because are not working 
> right now.


Thank you for looking into this; I'll start building diff14 to test locally.

I'm good with merging this checker as-is, as long as it is not enabled by 
default. I want to be able to use it even partially, in environments where I 
might not be able to inject a home-built compiler ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D72239: [clang-tidy] new opencl recursion not supported check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

The other recursion check seems more sophisticated. What is your take on it? 
Would you consider it better as well, what is your opinion on it?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D72239



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

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 237505.
njames93 added a comment.

- Fix perf alias documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-pre-increment.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-prefer-pre-increment.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment-disable-cpp-opcalls.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+template 
+class Iterator {
+  T *Current;
+
+public:
+  Iterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  Iterator ++() { return ++Current, *this; }
+  Iterator operator++(int) {
+Iterator Copy = *this;
+++Current;
+return Copy;
+  }
+  Iterator () { return --Current, *this; }
+  Iterator operator--(int) {
+Iterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+template 
+class PostfixIterator {
+  T *Current;
+
+public:
+  PostfixIterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  PostfixIterator operator++(int) {
+PostfixIterator Copy = *this;
+++Current;
+return Copy;
+  }
+  PostfixIterator operator--(int) {
+PostfixIterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+void foo() {
+  int Array[32];
+  Iterator It([0]);
+  It++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++It;
+  It--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --It;
+  (*It)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++(*It);
+  (*It)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --(*It);
+
+  *It++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  *It--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+
+  PostfixIterator PfIt([0]);
+  PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES-NOT: {{^}}  ++PfIt;
+  PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES-NOT: {{^}}  --PfIt;
+  (*PfIt)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++(*PfIt);
+  (*PfIt)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --(*PfIt);
+
+  *PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  *PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+}
Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+#define INC(X) X++
+#define DEC(X) X--
+
+void foo(int A) {
+  for (int I = 0; I < 10; I++) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-increment instead of Post-increment
+// CHECK-FIXES: {{^}}  for (int I = 0; I < 10; ++I) {
+  }
+  for (int I = 0; I < 10; ++I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  }
+  for (int I = 0; I < 10; A = I++) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  }
+
+  for (int I = 10; I < 0; I--) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-decrement instead of Post-decrement
+// CHECK-FIXES: {{^}}  for (int I = 10; I < 0; --I) {
+  }
+  for (int I = 10; I < 0; --I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of 

[PATCH] D72052: [UserManual] Update the C++ standard support

[Mark de Wever via Phabricator via cfe-commits]

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



Comment at: clang/docs/UsersManual.rst:2557
 
 clang fully implements all of standard C++98 except for exported
+templates (which were removed in C++11), and all of standards C++11, C++14,

xbolva00 wrote:
> Clang ? :)
I wondered about this before but we use both clang and Clang in the manual. So 
for now I decided not to make that change. If wanted I can make a separate 
patch doing the renaming. Should we then also change clang-cl to Clang-cl?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72052



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

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

In D72553#1815558 , @njames93 wrote:

> - 99% sure I have all incriments renamed now


I checked and can't find them anymore, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553



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

[clang] fce887b - GlobalModuleIndex - Fix use-after-move clang static analyzer warning.

[Simon Pilgrim via cfe-commits]

Author: Simon Pilgrim
Date: 2020-01-11T16:43:02Z
New Revision: fce887beb79780d0e0b19e8ab6176978a3dce9b8

URL: 
https://github.com/llvm/llvm-project/commit/fce887beb79780d0e0b19e8ab6176978a3dce9b8
DIFF: 
https://github.com/llvm/llvm-project/commit/fce887beb79780d0e0b19e8ab6176978a3dce9b8.diff

LOG: GlobalModuleIndex - Fix use-after-move clang static analyzer warning.

Shadow variable names meant we were referencing the Buffer input argument, not 
the GlobalModuleIndex member that its std::move()'d it.

Added: 


Modified: 
clang/lib/Serialization/GlobalModuleIndex.cpp

Removed: 




diff  --git a/clang/lib/Serialization/GlobalModuleIndex.cpp 
b/clang/lib/Serialization/GlobalModuleIndex.cpp
index 89a1c6cb8e69..462d29c2a0f1 100644
--- a/clang/lib/Serialization/GlobalModuleIndex.cpp
+++ b/clang/lib/Serialization/GlobalModuleIndex.cpp
@@ -125,11 +125,12 @@ typedef 
llvm::OnDiskIterableChainedHashTable
 
 }
 
-GlobalModuleIndex::GlobalModuleIndex(std::unique_ptr 
Buffer,
- llvm::BitstreamCursor Cursor)
-: Buffer(std::move(Buffer)), IdentifierIndex(), NumIdentifierLookups(),
+GlobalModuleIndex::GlobalModuleIndex(
+std::unique_ptr IndexBuffer,
+llvm::BitstreamCursor Cursor)
+: Buffer(std::move(IndexBuffer)), IdentifierIndex(), 
NumIdentifierLookups(),
   NumIdentifierLookupHits() {
-  auto Fail = [](llvm::Error &) {
+  auto Fail = [&](llvm::Error &) {
 report_fatal_error("Module index '" + Buffer->getBufferIdentifier() +
"' failed: " + toString(std::move(Err)));
   };



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@0x8000- i did remove `dependentTypes` from matching. Locally that works 
with your reported test-case (i was running clang-tidy wrong :().

I do understand the `auto &` issues now (i think).
If the type is deduced to a template-type or something that depends the 
standard ways of detecting that do not work, because in each template 
instantiation this information is not available.

Having a `auto & local = TemplatedVariable` does not have the 
`isInstantiationDependentType()`-bits set and for each instantiation of the 
template the const-analysis is run.
The false positives happened in different template instantiations, i think 
because the overloaded operators were sometimes `const` and sometimes not. I 
tried many ways to detect, if the type comes from a template, but had no 
success.
This is probably an issue in `Sema` or requires adjustments there.
I added tests in clang-tidy, but `#if 0` them out because are not working right 
now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D72052: [UserManual] Update the C++ standard support

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

xbolva00 added a comment.

+1




Comment at: clang/docs/UsersManual.rst:2557
 
 clang fully implements all of standard C++98 except for exported
+templates (which were removed in C++11), and all of standards C++11, C++14,

Clang ? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72052



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 237503.
JonasToth added a comment.

- fix false positive with ignoring dependentTypes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -62,13 +62,18 @@
   const auto *const S = selectFirst("stmt", Results);
   SmallVector Chain;
   ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+
+  std::string buffer;
   for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
 const Stmt *By = Analyzer.findMutation(E);
-std::string buffer;
+if (!By)
+  break;
+
 llvm::raw_string_ostream stream(buffer);
 By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
-Chain.push_back(StringRef(stream.str()).trim().str());
+Chain.emplace_back(StringRef(stream.str()).trim().str());
 E = dyn_cast(By);
+buffer.clear();
   }
   return Chain;
 }
@@ -382,22 +387,26 @@
   "void g(const int&&); void f() { int x; g(static_cast(x)); }");
   auto Results =
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 
   AST = buildASTFromCode("struct A {}; A operator+(const A&&, int);"
  "void f() { A x; static_cast(x) + 1; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 
   AST = buildASTFromCode("void f() { struct A { A(const int&&); }; "
  "int x; A y(static_cast(x)); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 
   AST = buildASTFromCode("void f() { struct A { A(); A(const A&&); }; "
  "A x; A y(static_cast(x)); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 }
 
 TEST(ExprMutationAnalyzerTest, Move) {
@@ -567,7 +576,7 @@
   const auto Results =
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
-  ElementsAre("return static_cast(x);"));
+  ElementsAre("static_cast(x)"));
 }
 
 TEST(ExprMutationAnalyzerTest, ReturnAsConstRRef) {
@@ -575,7 +584,8 @@
   "const int&& f() { int x; return static_cast(x); }");
   const auto Results =
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 }
 
 TEST(ExprMutationAnalyzerTest, TakeAddress) {
@@ -851,13 +861,13 @@
   auto Results =
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
-  ElementsAre("static_cast(x) = 10"));
+  ElementsAre("static_cast(x)"));
 
   AST = buildASTFromCode("typedef int& IntRef;"
  "void f() { int x; static_cast(x) = 10; }");
   Results = match(withEnclosingCompound(declRefTo("x")), 

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-prefer-pre-increment.rst:8
+
+The cert-oop54-cpp check is an alias, please see
+`llvm-prefer-pre-increment `_

Please fix check name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553



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

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 237502.
njames93 added a comment.

- 99% sure I have all incriments renamed now


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-pre-increment.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-prefer-pre-increment.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment-disable-cpp-opcalls.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+template 
+class Iterator {
+  T *Current;
+
+public:
+  Iterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  Iterator ++() { return ++Current, *this; }
+  Iterator operator++(int) {
+Iterator Copy = *this;
+++Current;
+return Copy;
+  }
+  Iterator () { return --Current, *this; }
+  Iterator operator--(int) {
+Iterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+template 
+class PostfixIterator {
+  T *Current;
+
+public:
+  PostfixIterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  PostfixIterator operator++(int) {
+PostfixIterator Copy = *this;
+++Current;
+return Copy;
+  }
+  PostfixIterator operator--(int) {
+PostfixIterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+void foo() {
+  int Array[32];
+  Iterator It([0]);
+  It++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++It;
+  It--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --It;
+  (*It)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++(*It);
+  (*It)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --(*It);
+
+  *It++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  *It--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+
+  PostfixIterator PfIt([0]);
+  PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES-NOT: {{^}}  ++PfIt;
+  PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES-NOT: {{^}}  --PfIt;
+  (*PfIt)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++(*PfIt);
+  (*PfIt)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --(*PfIt);
+
+  *PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  *PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+}
Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+#define INC(X) X++
+#define DEC(X) X--
+
+void foo(int A) {
+  for (int I = 0; I < 10; I++) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-increment instead of Post-increment
+// CHECK-FIXES: {{^}}  for (int I = 0; I < 10; ++I) {
+  }
+  for (int I = 0; I < 10; ++I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  }
+  for (int I = 0; I < 10; A = I++) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  }
+
+  for (int I = 10; I < 0; I--) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-decrement instead of Post-decrement
+// CHECK-FIXES: {{^}}  for (int I = 10; I < 0; --I) {
+  }
+  for (int I = 10; I < 0; --I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead 

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/PreferPreincrementCheck.cpp:38
+  // returned. Reordering those would change the behaviour of the expression.
+  // FIXME: Add any more parents which could use the result of the operation.
+  auto BadParents =

Mordante wrote:
> Does the checker give a lot of false positives?
First time I tried it there were lots of false positives. But after adding all 
those Stmt checks it appears to do a rather good job


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 237501.
JonasToth added a comment.

- remove pointee-analysis artifacts as this will be done later
- remove unnecessary comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -62,13 +62,18 @@
   const auto *const S = selectFirst("stmt", Results);
   SmallVector Chain;
   ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+
+  std::string buffer;
   for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
 const Stmt *By = Analyzer.findMutation(E);
-std::string buffer;
+if (!By)
+  break;
+
 llvm::raw_string_ostream stream(buffer);
 By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
-Chain.push_back(StringRef(stream.str()).trim().str());
+Chain.emplace_back(StringRef(stream.str()).trim().str());
 E = dyn_cast(By);
+buffer.clear();
   }
   return Chain;
 }
@@ -382,22 +387,26 @@
   "void g(const int&&); void f() { int x; g(static_cast(x)); }");
   auto Results =
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 
   AST = buildASTFromCode("struct A {}; A operator+(const A&&, int);"
  "void f() { A x; static_cast(x) + 1; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 
   AST = buildASTFromCode("void f() { struct A { A(const int&&); }; "
  "int x; A y(static_cast(x)); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 
   AST = buildASTFromCode("void f() { struct A { A(); A(const A&&); }; "
  "A x; A y(static_cast(x)); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 }
 
 TEST(ExprMutationAnalyzerTest, Move) {
@@ -567,7 +576,7 @@
   const auto Results =
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
-  ElementsAre("return static_cast(x);"));
+  ElementsAre("static_cast(x)"));
 }
 
 TEST(ExprMutationAnalyzerTest, ReturnAsConstRRef) {
@@ -575,7 +584,8 @@
   "const int&& f() { int x; return static_cast(x); }");
   const auto Results =
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+  ElementsAre("static_cast(x)"));
 }
 
 TEST(ExprMutationAnalyzerTest, TakeAddress) {
@@ -851,13 +861,13 @@
   auto Results =
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
-  ElementsAre("static_cast(x) = 10"));
+  ElementsAre("static_cast(x)"));
 
   AST = buildASTFromCode("typedef int& IntRef;"
  "void f() { int x; static_cast(x) = 10; }");
   Results = 

[clang] ded237b - Fix "pointer is null" static analyzer warning. NFCI.

[Simon Pilgrim via cfe-commits]

Author: Simon Pilgrim
Date: 2020-01-11T16:02:22Z
New Revision: ded237b58d56299f90ef44853ef79b039248b85e

URL: 
https://github.com/llvm/llvm-project/commit/ded237b58d56299f90ef44853ef79b039248b85e
DIFF: 
https://github.com/llvm/llvm-project/commit/ded237b58d56299f90ef44853ef79b039248b85e.diff

LOG: Fix "pointer is null" static analyzer warning. NFCI.

Use castAs<> instead of getAs<> since the pointer is dereferenced immediately 
below and castAs will perform the null assertion for us.

Added: 


Modified: 
clang/lib/Sema/SemaDecl.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 468db9cfc7f8..507e4a6cd436 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13936,8 +13936,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt 
*Body,
 LSI->ReturnType.isNull() ? Context.VoidTy : LSI->ReturnType;
 
 // Update the return type to the deduced type.
-const FunctionProtoType *Proto =
-FD->getType()->getAs();
+const auto *Proto = FD->getType()->castAs();
 FD->setType(Context.getFunctionType(RetType, Proto->getParamTypes(),
 Proto->getExtProtoInfo()));
   }



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

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 237500.
njames93 added a comment.

- Renamed incriment in test cases to increment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-pre-increment.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-prefer-pre-increment.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment-disable-cpp-opcalls.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+template 
+class Iterator {
+  T *Current;
+
+public:
+  Iterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  Iterator ++() { return ++Current, *this; }
+  Iterator operator++(int) {
+Iterator Copy = *this;
+++Current;
+return Copy;
+  }
+  Iterator () { return --Current, *this; }
+  Iterator operator--(int) {
+Iterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+template 
+class PostfixIterator {
+  T *Current;
+
+public:
+  PostfixIterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  PostfixIterator operator++(int) {
+PostfixIterator Copy = *this;
+++Current;
+return Copy;
+  }
+  PostfixIterator operator--(int) {
+PostfixIterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+void foo() {
+  int Array[32];
+  Iterator It([0]);
+  It++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++It;
+  It--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --It;
+  (*It)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++(*It);
+  (*It)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --(*It);
+
+  *It++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  *It--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+
+  PostfixIterator PfIt([0]);
+  PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES-NOT: {{^}}  ++PfIt;
+  PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES-NOT: {{^}}  --PfIt;
+  (*PfIt)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-increment instead of Post-increment
+  // CHECK-FIXES: {{^}}  ++(*PfIt);
+  (*PfIt)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decrement instead of Post-decrement
+  // CHECK-FIXES: {{^}}  --(*PfIt);
+
+  *PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  *PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead of Post-decrement
+}
Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+#define INC(X) X++
+#define DEC(X) X--
+
+void foo(int A) {
+  for (int I = 0; I < 10; I++) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-increment instead of Post-increment
+// CHECK-FIXES: {{^}}  for (int I = 0; I < 10; ++I) {
+  }
+  for (int I = 0; I < 10; ++I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  }
+  for (int I = 0; I < 10; A = I++) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-increment instead of Post-increment
+  }
+
+  for (int I = 10; I < 0; I--) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-decrement instead of Post-decrement
+// CHECK-FIXES: {{^}}  for (int I = 10; I < 0; --I) {
+  }
+  for (int I = 10; I < 0; --I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decrement instead 

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 237499.
njames93 marked 2 inline comments as done.
njames93 added a comment.

- Added alias for performance module
- renamed check to prefer-pre-increment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreIncrementCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-pre-increment.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-prefer-pre-increment.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment-disable-cpp-opcalls.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+template 
+class Iterator {
+  T *Current;
+
+public:
+  Iterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  Iterator ++() { return ++Current, *this; }
+  Iterator operator++(int) {
+Iterator Copy = *this;
+++Current;
+return Copy;
+  }
+  Iterator () { return --Current, *this; }
+  Iterator operator--(int) {
+Iterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+template 
+class PostfixIterator {
+  T *Current;
+
+public:
+  PostfixIterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  PostfixIterator operator++(int) {
+PostfixIterator Copy = *this;
+++Current;
+return Copy;
+  }
+  PostfixIterator operator--(int) {
+PostfixIterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+void foo() {
+  int Array[32];
+  Iterator It([0]);
+  It++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-incriment instead of Post-incriment
+  // CHECK-FIXES: {{^}}  ++It;
+  It--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decriment instead of Post-decriment
+  // CHECK-FIXES: {{^}}  --It;
+  (*It)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-incriment instead of Post-incriment
+  // CHECK-FIXES: {{^}}  ++(*It);
+  (*It)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decriment instead of Post-decriment
+  // CHECK-FIXES: {{^}}  --(*It);
+
+  *It++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  *It--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decriment instead of Post-decriment
+
+  PostfixIterator PfIt([0]);
+  PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  // CHECK-FIXES-NOT: {{^}}  ++PfIt;
+  PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decriment instead of Post-decriment
+  // CHECK-FIXES-NOT: {{^}}  --PfIt;
+  (*PfIt)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-incriment instead of Post-incriment
+  // CHECK-FIXES: {{^}}  ++(*PfIt);
+  (*PfIt)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decriment instead of Post-decriment
+  // CHECK-FIXES: {{^}}  --(*PfIt);
+
+  *PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  *PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decriment instead of Post-decriment
+}
Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-pre-increment.c
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s llvm-prefer-pre-increment %t
+
+#define INC(X) X++
+#define DEC(X) X--
+
+void foo(int A) {
+  for (int I = 0; I < 10; I++) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-incriment instead of Post-incriment
+// CHECK-FIXES: {{^}}  for (int I = 0; I < 10; ++I) {
+  }
+  for (int I = 0; I < 10; ++I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  }
+  for (int I = 0; I < 10; A = I++) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  }
+
+  for (int I = 10; I < 0; I--) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-decriment instead of Post-decriment
+// CHECK-FIXES: {{^}}  for (int I = 10; I < 0; --I) {
+  }
+  for (int I = 10; I < 0; --I) {
+// 

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

I only have some naming nits

- incriment -> increment
- decriment -> decrement

Maybe rename the check llvm-prefer-preincrement -> llvm-prefer-pre-increment
and rename the files and classes PreferPreincrementCheck -> 
PreferPreIncrementCheck




Comment at: clang-tools-extra/clang-tidy/llvm/PreferPreincrementCheck.cpp:38
+  // returned. Reordering those would change the behaviour of the expression.
+  // FIXME: Add any more parents which could use the result of the operation.
+  auto BadParents =

Does the checker give a lot of false positives?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553



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

[PATCH] D72052: [UserManual] Update the C++ standard support

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

Friendly ping, it would be nice to get this in the next release.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72052



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

[PATCH] D72378: [clang-tidy] Add `bugprone-reserved-identifier`

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

Adding `Depends on D72284` would add the patches to the stack. 
I like the patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72378



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

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D72553#1815497 , @njames93 wrote:

> could this do with an alias into performance?


Sure. At least such tricky questions were asked on interviews decade ago :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1815473 , @JonasToth wrote:

> In D54943#1815358 , @0x8000- 
> wrote:
>
> > Here is a minimal example that shows the problem:
>
>
> I can not reproduce that case :(
>  It gives me a compilation error in `const DoGooder anInstanceOf...`, because 
> `DoGooder` requires an template parameter. I then add `DoGooder` and the 
> variable gets no transformation.
>  Did you reproduce the error with exactly that code?
>
> And which version did you run? Maybe that was a previous false positive, that 
> might be fixed right now?


The way I am testing is that I am fetching the current master branch of 
https://github.com/llvm/llvm-project.git, then downloading the latest patch 
from here and applying on top, then building.

This is the state of the tree right as of last night:

  commit 1c8ab48028cc39429a392691ef2e66968460d782 (HEAD -> D54943.diff12)
  Author: Florin Iucha 
  Date:   Fri Jan 10 18:52:54 2020 -0500
  
  D54943.diff12
  
  commit 1b8c84b8dd5a4a294943a6a6f0631d2d3a1f9f27 (origin/master, origin/HEAD, 
master)
  Author: Richard Smith 
  Date:   Fri Jan 10 15:47:29 2020 -0800
  
  Improve precision of documentation comment.

CTAD takes care of the missing template parameter, that's why I'm passing 
"-std=c++17" to clang-tidy.

  /tmp$ /opt/clang10/bin/clang-tidy 
-checks="-*,cppcoreguidelines-const-correctness" -header-filter=".*" test.cpp 
-- -std=c++17 -Wall
  54 warnings generated.
  /tmp/test.cpp:22:9: warning: variable 'anInstanceOf' of type '' can be declared 'const' [cppcoreguidelines-const-correctness]
  const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
  ^
 const 
  Suppressed 53 warnings (53 in non-user code).
  Use -header-filter=.* to display errors from all non-system headers. Use 
-system-headers to display errors from system headers as well.
  /tmp$ /opt/clang10/bin/clang++ -c test.cpp
  test.cpp:22:15: error: use of class template 'DoGooder' requires template 
arguments
  const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
^
  test.cpp:4:8: note: template is declared here
  struct DoGooder
 ^
  1 error generated.
  /tmp$ /opt/clang10/bin/clang++ -std=c++17 -c test.cpp
  -rw-r--r-- 1 florin florin 432 Jan 11 09:48 test.cpp
  -rw-r--r-- 1 florin florin 744 Jan 11 09:52 test.o


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



___
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

[Mark de Wever via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Mordante marked 3 inline comments as done.
Closed by commit rG9c74fb402e1b: [Sema] Improve -Wrange-loop-analysis warnings. 
(authored by Mordante).

Changed prior to commit:
  https://reviews.llvm.org/D72212?vs=236199=237497#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72212

Files:
  clang/lib/Sema/SemaStmt.cpp
  clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.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
@@ -20,6 +20,10 @@
 
 struct Foo {};
 struct Bar {
+  // Small trivially copyable types do not show a warning when copied in a
+  // range-based for loop. This size ensures the object is not considered
+  // small.
+  char s[128];
   Bar(Foo);
   Bar(int);
   operator int();
Index: clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wloop-analysis -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wrange-loop-analysis -verify %s
+
+void test_POD_64_bytes() {
+  struct Record {
+char a[64];
+  };
+
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_POD_65_bytes() {
+  struct Record {
+char a[65];
+  };
+
+  // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+  // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_TriviallyCopyable_64_bytes() {
+  struct Record {
+Record() {}
+char a[64];
+  };
+
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_TriviallyCopyable_65_bytes() {
+  struct Record {
+Record() {}
+char a[65];
+  };
+
+  // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+  // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_NonTriviallyCopyable() {
+  struct Record {
+Record() {}
+~Record() {}
+volatile int a;
+int b;
+  };
+
+  // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+  // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_TrivialABI_64_bytes() {
+  struct [[clang::trivial_abi]] Record {
+Record() {}
+~Record() {}
+char a[64];
+  };
+
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_TrivialABI_65_bytes() {
+  struct [[clang::trivial_abi]] Record {
+Record() {}
+~Record() {}
+char a[65];
+  };
+
+  // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+  // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -2779,6 +2779,15 @@
   }
 }
 
+/// Determines whether the @p VariableType's declaration is a record with the
+/// clang::trivial_abi attribute.
+static bool hasTrivialABIAttr(QualType VariableType) {
+  if (CXXRecordDecl *RD = VariableType->getAsCXXRecordDecl())
+return RD->hasAttr();
+
+  return false;
+}
+
 // Warns when the loop variable can be changed to a reference type to
 // prevent a copy.  For instance, if given "for (const Foo x : Range)" suggest
 // "for (const Foo  : Range)" if this form does not make a copy.
@@ -2800,10 +2809,13 @@
 return;
   }
 
-  // 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. 64 bytes is a common size of a cache line.
+  // (The function `getTypeSize` returns the size in bits.)
+  ASTContext  = SemaRef.Context;
+  if (Ctx.getTypeSize(VariableType) <= 64 * 8 &&
+  (VariableType.isTriviallyCopyableType(Ctx) ||
+   hasTrivialABIAttr(VariableType)))
 return;
 
   // Suggest changing from a const variable to a const reference variable

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

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 8 inline comments as done.
Mordante 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;

rtrieu wrote:
> 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.
I was not sure whether we also wanted this information in the comment. I'll add 
more information to the comment.



Comment at: clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp:6
+  struct Record {
+volatile int a;
+int b;

MaskRay wrote:
> `test_POD` can be dropped. It does not add test coverage.
> 
> "Struct with a volatile member no longer a POD" is a MSVC bug. 
> https://stackoverflow.com/questions/59103157/struct-with-a-volatile-member-no-longer-a-pod-according-to-msvc
I'll remove these tests. I think there are other tests that test what is and is 
not a POD.



Comment at: clang/test/SemaCXX/warn-range-loop-analysis.cpp:22
 struct Bar {
+  // The type is too large to suppress the warning for trivially copyable 
types.
+  char s[128];

MaskRay wrote:
> `too large to suppress` means it does not suppress the warning in English, I 
> think.
I'll clarify the text.


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

[clang] 9c74fb4 - [Sema] Improve -Wrange-loop-analysis warnings.

[Mark de Wever via cfe-commits]

Author: Mark de Wever
Date: 2020-01-11T15:34:02+01:00
New Revision: 9c74fb402e1b7aad4a509a49ab4792154b8ba2c8

URL: 
https://github.com/llvm/llvm-project/commit/9c74fb402e1b7aad4a509a49ab4792154b8ba2c8
DIFF: 
https://github.com/llvm/llvm-project/commit/9c74fb402e1b7aad4a509a49ab4792154b8ba2c8.diff

LOG: [Sema] Improve -Wrange-loop-analysis warnings.

No longer generate a diagnostic when a small trivially copyable type is
used without a reference. Before the test looked for a POD type and had no
size restriction. Since the range-based for loop is only available in
C++11 and POD types are trivially copyable in C++11 it's not required to
test for a POD type.

Since copying a large object will be expensive its size has been
restricted. 64 bytes is a common size of a cache line and if the object is
aligned the copy will be cheap. No performance impact testing has been
done.

Differential Revision: https://reviews.llvm.org/D72212

Added: 
clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp

Modified: 
clang/lib/Sema/SemaStmt.cpp
clang/test/SemaCXX/warn-range-loop-analysis.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 1e7cc503d8c2..d6c3af9e84c8 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -2779,6 +2779,15 @@ static void DiagnoseForRangeReferenceVariableCopies(Sema 
,
   }
 }
 
+/// Determines whether the @p VariableType's declaration is a record with the
+/// clang::trivial_abi attribute.
+static bool hasTrivialABIAttr(QualType VariableType) {
+  if (CXXRecordDecl *RD = VariableType->getAsCXXRecordDecl())
+return RD->hasAttr();
+
+  return false;
+}
+
 // Warns when the loop variable can be changed to a reference type to
 // prevent a copy.  For instance, if given "for (const Foo x : Range)" suggest
 // "for (const Foo  : Range)" if this form does not make a copy.
@@ -2800,10 +2809,13 @@ static void DiagnoseForRangeConstVariableCopies(Sema 
,
 return;
   }
 
-  // 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. 64 bytes is a common size of a cache line.
+  // (The function `getTypeSize` returns the size in bits.)
+  ASTContext  = SemaRef.Context;
+  if (Ctx.getTypeSize(VariableType) <= 64 * 8 &&
+  (VariableType.isTriviallyCopyableType(Ctx) ||
+   hasTrivialABIAttr(VariableType)))
 return;
 
   // Suggest changing from a const variable to a const reference variable

diff  --git 
a/clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp 
b/clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
new file mode 100644
index ..f4c76f26f5c9
--- /dev/null
+++ b/clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wloop-analysis -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wrange-loop-analysis -verify %s
+
+void test_POD_64_bytes() {
+  struct Record {
+char a[64];
+  };
+
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_POD_65_bytes() {
+  struct Record {
+char a[65];
+  };
+
+  // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a 
copy from type 'const Record'}}
+  // expected-note@+2 {{use reference type 'const Record &' to prevent 
copying}}
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_TriviallyCopyable_64_bytes() {
+  struct Record {
+Record() {}
+char a[64];
+  };
+
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_TriviallyCopyable_65_bytes() {
+  struct Record {
+Record() {}
+char a[65];
+  };
+
+  // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a 
copy from type 'const Record'}}
+  // expected-note@+2 {{use reference type 'const Record &' to prevent 
copying}}
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_NonTriviallyCopyable() {
+  struct Record {
+Record() {}
+~Record() {}
+volatile int a;
+int b;
+  };
+
+  // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a 
copy from type 'const Record'}}
+  // expected-note@+2 {{use reference type 'const Record &' to prevent 
copying}}
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_TrivialABI_64_bytes() {
+  struct [[clang::trivial_abi]] Record {
+Record() {}
+~Record() {}
+char a[64];
+  };
+
+  Record records[8];
+  for (const auto r : records)
+(void)r;
+}
+
+void test_TrivialABI_65_bytes() {
+  struct [[clang::trivial_abi]] Record {
+Record() {}
+

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

Ping @MyDeveloperDay @klimek


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

https://reviews.llvm.org/D71512



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

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[liu hui via Phabricator via cfe-commits]

lh123 added a comment.

In D72498#1813902 , @ilya-biryukov 
wrote:

> Could it be the case that we want to show the canonical types (i.e. without 
> all syntax sugar)?
>  Maybe we want both the normal type and the canonical type?


+1,I think it would be helpful to show the canonical type in this case.

  int main() {
  std::vector a;
  a.front(); // hover on front
  }

hover over the `front` , you'll see "instance-method `front` → 
`std::vector >::reference`".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



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

[PATCH] D72488: [clang-tidy] Add check for CERT-OOP57-CPP

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 237492.
njames93 marked an inline comment as done.
njames93 added a comment.

Figured out what memset you meant...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72488

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp
  clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-oop57-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-oop57-cpp.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-oop57-cpp.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s cert-oop57-cpp %t -- \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cert-oop57-cpp.MemSetNames, value: mymemset}, \
+// RUN:  {key: cert-oop57-cpp.MemCpyNames, value: mymemcpy}, \
+// RUN:  {key: cert-oop57-cpp.MemCmpNames, value: mymemcmp}]}' \
+// RUN: --
+
+void mymemset(void *, unsigned char, decltype(sizeof(int)));
+void mymemcpy(void *, const void *, decltype(sizeof(int)));
+int mymemcmp(const void *, const void *, decltype(sizeof(int)));
+
+namespace std {
+void memset(void *, unsigned char, decltype(sizeof(int)));
+void memcpy(void *, const void *, decltype(sizeof(int)));
+void memmove(void *, const void *, decltype(sizeof(int)));
+void strcpy(void *, const void *, decltype(sizeof(int)));
+int memcmp(const void *, const void *, decltype(sizeof(int)));
+int strcmp(const void *, const void *, decltype(sizeof(int)));
+} // namespace std
+
+struct Trivial {
+  int I;
+  int J;
+};
+
+struct NonTrivial {
+  int I;
+  int J;
+
+  NonTrivial() : I(0), J(0) {}
+  NonTrivial =(const NonTrivial ) {
+I = Other.I;
+J = Other.J;
+return *this;
+  }
+
+  bool operator==(const Trivial ) const {
+return I == Other.I && J == Other.J;
+  }
+  bool operator!=(const Trivial ) const {
+return !(*this == Other);
+  }
+};
+
+void foo(const Trivial ) {
+  Trivial Data;
+  std::memset(, 0, sizeof(Data));
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: Calling 'std::memset' on a non trivially default constructible class is undefined
+  std::memset(, 0, sizeof(Trivial));
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: Calling 'std::memset' on a non trivially default constructible class is undefined
+  std::memcpy(, , sizeof(Data));
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: Calling 'std::memcpy' on a non trivially copyable class is undefined
+  std::memmove(, , sizeof(Data));
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: Calling 'std::memmove' on a non trivially copyable class is undefined
+  std::strcpy(, , sizeof(Data));
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: Calling 'std::strcpy' on a non trivially copyable class is undefined
+  std::memcmp(, , sizeof(Data));
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: consider using comparison operators instead of calling 'std::memcmp'
+  std::strcmp(, , sizeof(Data));
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: consider using comparison operators instead of calling 'std::strcmp'
+}
+
+void bar(const NonTrivial ) {
+  NonTrivial Data;
+  std::memset(, 0, sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Calling 'std::memset' on a non trivially default constructible class is undefined
+  // Check it detects sizeof(Type) as well as sizeof(Instantiation)
+  std::memset(, 0, sizeof(NonTrivial));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Calling 'std::memset' on a non trivially default constructible class is undefined
+  std::memcpy(, , sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Calling 'std::memcpy' on a non trivially copyable class is undefined
+  std::memmove(, , sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Calling 'std::memmove' on a non trivially copyable class is undefined
+  std::strcpy(, , sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Calling 'std::strcpy' on a non trivially copyable class is undefined
+  std::memcmp(, , sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: consider using comparison operators instead of calling 'std::memcmp'
+  std::strcmp(, , sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: consider using comparison operators instead of calling 'std::strcmp'
+}
+
+void baz(const NonTrivial ) {
+  NonTrivial Data;
+  mymemset(, 0, sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Calling 'mymemset' on a non trivially default constructible class is undefined
+  mymemcpy(, , sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Calling 'mymemcpy' on a non trivially 

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Nathan James via Phabricator via cfe-commits]

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

could this do with an alias into performance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553



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

[PATCH] D72227: [Driver][X86] Add -malign-branch* and -malign-branch-within-32B-boundaries

[Kan Shengchen via Phabricator via cfe-commits]

skan updated this revision to Diff 237491.
skan retitled this revision from "Add options for clang to align branches 
within 32B boundary" to "[Driver][X86] Add -malign-branch* and 
-malign-branch-within-32B-boundaries".
skan edited the summary of this revision.

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

https://reviews.llvm.org/D72227

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/x86-malign-branch.c
  clang/test/Driver/x86-malign-branch.s

Index: clang/test/Driver/x86-malign-branch.s
===
--- /dev/null
+++ clang/test/Driver/x86-malign-branch.s
@@ -0,0 +1,13 @@
+/// Test that -malign-branch* are handled for assembly files.
+
+// RUN: %clang -target x86_64 -malign-branch-boundary=16 %s -c -### 2>&1 | FileCheck %s --check-prefix=BOUNDARY
+// BOUNDARY: "-mllvm" "-x86-align-branch-boundary=16"
+
+// RUN: %clang -target x86_64 -malign-branch=fused,jcc,jmp %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE
+// TYPE: "-mllvm" "-x86-align-branch=fused+jcc+jmp"
+
+// RUN: %clang -target x86_64 -malign-branch-prefix-size=5 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX
+// PREFIX: "-mllvm" "-x86-align-branch-prefix-size=5"
+
+// RUN: %clang -target x86_64 -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=32B
+// 32B: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=5"
Index: clang/test/Driver/x86-malign-branch.c
===
--- /dev/null
+++ clang/test/Driver/x86-malign-branch.c
@@ -0,0 +1,44 @@
+/// Test that -malign-branch* options are parsed and converted to -mllvm options.
+
+/// Test -malign-branch-boundary=
+// RUN: %clang -target x86_64 -malign-branch-boundary=16 %s -c -### 2>&1 | FileCheck %s --check-prefix=BOUNDARY
+// BOUNDARY: "-mllvm" "-x86-align-branch-boundary=16"
+
+// RUN: %clang -target x86_64 -malign-branch-boundary=8 %s -c -### 2>&1 | FileCheck %s --check-prefix=BOUNDARY-ERR
+// RUN: %clang -target x86_64 -malign-branch-boundary=15 %s -c -### 2>&1 | FileCheck %s --check-prefix=BOUNDARY-ERR
+// BOUNDARY-ERR: invalid argument {{.*}} to -malign-branch-boundary=
+
+/// Test -malign-branch=
+// RUN: %clang -target x86_64 -malign-branch=fused,jcc,jmp %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE0
+// TYPE0: "-mllvm" "-x86-align-branch=fused+jcc+jmp"
+// RUN: %clang -target x86_64 -malign-branch=fused,jcc,jmp,ret,call,indirect %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE1
+// TYPE1: "-mllvm" "-x86-align-branch=fused+jcc+jmp+ret+call+indirect"
+
+// RUN: %clang -target x86_64 -malign-branch=fused,foo,bar %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE-ERR
+// TYPE-ERR: invalid argument 'foo' to -malign-branch=; each element must be one of: fused, jcc, jmp, call, ret, indirect
+// TYPE-ERR: invalid argument 'bar' to -malign-branch=; each element must be one of: fused, jcc, jmp, call, ret, indirect
+
+/// Test -malign-branch-prefix-size=
+// RUN: %clang -target x86_64 -malign-branch-prefix-size=0 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-0
+// PREFIX-0: "-mllvm" "-x86-align-branch-prefix-size=0"
+// RUN: %clang -target x86_64 -malign-branch-prefix-size=5 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-5
+// PREFIX-5: "-mllvm" "-x86-align-branch-prefix-size=5"
+
+// RUN: %clang -target x86_64 -malign-branch-prefix-size=6 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-6
+// PREFIX-6: invalid argument
+
+/// Test -mbranches-within-32B-boundaries
+// RUN: %clang -target x86_64 -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=32B
+// 32B: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=5"
+/// Test that -malign-branch* can -mbranches-within-32B-boundaries can override each other
+// RUN: %clang -target x86_64 -malign-branch=jcc -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=32B-TYPE1
+// 32B-TYPE1:  "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=5"
+// RUN: %clang -target x86_64 -mbranches-within-32B-boundaries -malign-branch=jcc %s -c -### 2>&1 | FileCheck %s --check-prefix=32B-TYPE2
+// 32B-TYPE2:  "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=jcc" "-mllvm" "-x86-align-branch-prefix-size=5"
+
+/// Unsupported on other targets.
+// RUN: %clang -target aarch64 -malign-branch=jmp %s -c -### 2>&1 | FileCheck --check-prefix=UNUSED %s
+// RUN: %clang -target aarch64 -malign-branch-boundary=7 %s -c -### 2>&1 | FileCheck --check-prefix=UNUSED %s
+// RUN: %clang -target aarch64 -malign-branch-prefix-size=15 %s -c -### 2>&1 | FileCheck --check-prefix=UNUSED %s
+// RUN: %clang 

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 237490.
njames93 added a comment.

- Small refactor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreincrementCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferPreincrementCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-preincrement.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-preincrement-disable-cpp-opcalls.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-preincrement.c
  clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-preincrement.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-preincrement.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-preincrement.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s llvm-prefer-preincrement %t
+
+template 
+class Iterator {
+  T *Current;
+
+public:
+  Iterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  Iterator ++() { return ++Current, *this; }
+  Iterator operator++(int) {
+Iterator Copy = *this;
+++Current;
+return Copy;
+  }
+  Iterator () { return --Current, *this; }
+  Iterator operator--(int) {
+Iterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+template 
+class PostfixIterator {
+  T *Current;
+
+public:
+  PostfixIterator(T *Pointer) : Current(Pointer) {}
+  T *() const { return *Current; }
+  PostfixIterator operator++(int) {
+PostfixIterator Copy = *this;
+++Current;
+return Copy;
+  }
+  PostfixIterator operator--(int) {
+PostfixIterator Copy = *this;
+--Current;
+return Copy;
+  }
+};
+
+void foo() {
+  int Array[32];
+  Iterator It([0]);
+  It++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-incriment instead of Post-incriment
+  // CHECK-FIXES: {{^}}  ++It;
+  It--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decriment instead of Post-decriment
+  // CHECK-FIXES: {{^}}  --It;
+  (*It)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-incriment instead of Post-incriment
+  // CHECK-FIXES: {{^}}  ++(*It);
+  (*It)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decriment instead of Post-decriment
+  // CHECK-FIXES: {{^}}  --(*It);
+
+  *It++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  *It--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decriment instead of Post-decriment
+
+  PostfixIterator PfIt([0]);
+  PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  // CHECK-FIXES-NOT: {{^}}  ++PfIt;
+  PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decriment instead of Post-decriment
+  // CHECK-FIXES-NOT: {{^}}  --PfIt;
+  (*PfIt)++;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-incriment instead of Post-incriment
+  // CHECK-FIXES: {{^}}  ++(*PfIt);
+  (*PfIt)--;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Use Pre-decriment instead of Post-decriment
+  // CHECK-FIXES: {{^}}  --(*PfIt);
+
+  *PfIt++;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  *PfIt--;
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decriment instead of Post-decriment
+}
Index: clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-preincrement.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-preincrement.c
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s llvm-prefer-preincrement %t
+
+#define INC(X) X++
+#define DEC(X) X--
+
+void foo(int A) {
+  for (int I = 0; I < 10; I++) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-incriment instead of Post-incriment
+// CHECK-FIXES: {{^}}  for (int I = 0; I < 10; ++I) {
+  }
+  for (int I = 0; I < 10; ++I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  }
+  for (int I = 0; I < 10; A = I++) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-incriment instead of Post-incriment
+  }
+
+  for (int I = 10; I < 0; I--) {
+// CHECK-MESSAGES: [[@LINE-1]]:27: warning: Use Pre-decriment instead of Post-decriment
+// CHECK-FIXES: {{^}}  for (int I = 10; I < 0; --I) {
+  }
+  for (int I = 10; I < 0; --I) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decriment instead of Post-decriment
+  }
+  for (int I = 10; I < 0; A = I--) {
+// CHECK-MESSAGES-NOT: [[@LINE-1]]:{{\d*}}: warning: Use Pre-decriment instead of Post-decriment
+  }
+
+  for (int I = 0; 

[PATCH] D72401: Fixes for spaces around C# object initializers

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/unittests/Format/FormatTestCSharp.cpp:379
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
+

Is this needed for the formatting below? I find it surprising if this controls 
the spacing before braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72401



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

[PATCH] D72401: Fixes for spaces around C# object initializers

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir requested changes to this revision.
krasimir added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2869
+// space before '{' in "new MyType {"
+if (Left.is(TT_Unknown) && Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))

This test feels a bit too rigid: you might additionally want to consider `new 
Type {` and `new Type /*comment*/ {` and `new [] /* comment */ {`.
For these you might find the `MatchingParen` and `getPreviousNonComment` 
useful. And example for this is below, at line 2880-2885 in javascript handling.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2870
+if (Left.is(TT_Unknown) && Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;

The `TT_Unknown` is a bit confusing -- why is it needed? If this is a 
workaround for something, please add it as a comment. We might need to improve 
the token detection later to allow for more complicated pattern matching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72401



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

[PATCH] D72144: Treat C# `using` as a control statement

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir requested changes to this revision.
krasimir added inline comments.
This revision now requires changes to proceed.



Comment at: clang/unittests/Format/FormatTestCSharp.cpp:257
+   "}",
+   Style);
+

Please also add a test for `SBPO_NonEmptyParentheses`.



Comment at: clang/unittests/Format/FormatTestCSharp.cpp:259
+
+  verifyFormat("using (StreamWriter sw = new StreamWriter(filenameB)) {}",
+   Style);

Can this appear at the top-level? If not, just keep the other test case to 
avoid confusion.


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

https://reviews.llvm.org/D72144



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#1815358 , @0x8000- wrote:

> Here is a minimal example that shows the problem:
>
>   #include 
>  
>   template
>   struct DoGooder
>   {
>   DoGooder(void* accessor, SomeValue value)
>   {
>   }
>  
>   };
>  
>   struct Bingus
>   {
>   static constexpr auto someRandomConstant = 99;
>   };
>  
>   template
>   struct HardWorker
>   {
>   HardWorker()
>   {
>   const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
>   }
>   };
>  
>   struct TheContainer
>   {
>   std::unique_ptr> m_theOtherInstance;
>   };
>
>
> Example run:
>
>   $ /opt/clang10/bin/clang-tidy 
> -checks="-*,cppcoreguidelines-const-correctness" -header-filter=".*" 
> reproconst.cpp -- -std=c++17 -Wall
>   54 warnings generated.
>   reproconst.cpp:22:9: warning: variable 'anInstanceOf' of type ' type>' can be declared 'const' [cppcoreguidelines-const-correctness]
>   const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
>   ^
>  const
>   Suppressed 53 warnings (53 in non-user code).
>   Use -header-filter=.* to display errors from all non-system headers. Use 
> -system-headers to display errors from system headers as well.
>


I can not reproduce that case :(
It gives me a compilation error in `const DoGooder anInstanceOf...`, because 
`DoGooder` requires an template parameter. I then add `DoGooder` and the 
variable gets no transformation.
Did you reproduce the error with exactly that code?

And which version did you run? Maybe that was a previous false positive, that 
might be fixed right now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D68720: Support -fstack-clash-protection for x86

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 237486.
serge-sans-paille added a comment.

- update warn_fe_stack_clash_protection_inline_asm location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68720

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/stack-clash-protection.c
  clang/test/Driver/stack-clash-protection.c
  llvm/docs/ReleaseNotes.rst
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/lib/Target/X86/X86CallFrameOptimization.cpp
  llvm/lib/Target/X86/X86FrameLowering.cpp
  llvm/lib/Target/X86/X86FrameLowering.h
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86ISelLowering.h
  llvm/lib/Target/X86/X86InstrCompiler.td
  llvm/lib/Target/X86/X86InstrInfo.td
  llvm/test/CodeGen/X86/stack-clash-dynamic-alloca.ll
  llvm/test/CodeGen/X86/stack-clash-large.ll
  llvm/test/CodeGen/X86/stack-clash-medium-natural-probes-mutliple-objects.ll
  llvm/test/CodeGen/X86/stack-clash-medium-natural-probes.ll
  llvm/test/CodeGen/X86/stack-clash-medium.ll
  llvm/test/CodeGen/X86/stack-clash-no-free-probe.ll
  llvm/test/CodeGen/X86/stack-clash-small.ll
  llvm/test/CodeGen/X86/stack-clash-unknown-call.ll

Index: llvm/test/CodeGen/X86/stack-clash-unknown-call.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/stack-clash-unknown-call.ll
@@ -0,0 +1,31 @@
+; RUN: llc < %s | FileCheck %s
+
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg);
+
+define void @foo() local_unnamed_addr #0 {
+
+;CHECK-LABEL: foo:
+;CHECK: # %bb.0:
+;CHECK-NEXT:	subq	$4096, %rsp # imm = 0x1000
+; it's important that we don't use the call as a probe here
+;CHECK-NEXT:	movq	$0, (%rsp)
+;CHECK-NEXT:	subq	$3912, %rsp # imm = 0xF48
+;CHECK-NEXT:	.cfi_def_cfa_offset 8016
+;CHECK-NEXT:	movq	%rsp, %rdi
+;CHECK-NEXT:	movl	$8000, %edx # imm = 0x1F40
+;CHECK-NEXT:	xorl	%esi, %esi
+;CHECK-NEXT:	callq	memset
+;CHECK-NEXT:	addq	$8008, %rsp # imm = 0x1F48
+;CHECK-NEXT:	.cfi_def_cfa_offset 8
+;CHECK-NEXT:	retq
+
+  %a = alloca i8, i64 8000, align 16
+  call void @llvm.memset.p0i8.i64(i8* align 16 %a, i8 0, i64 8000, i1 false)
+  ret void
+}
+
+attributes #0 =  {"probe-stack"="inline-asm"}
Index: llvm/test/CodeGen/X86/stack-clash-small.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/stack-clash-small.ll
@@ -0,0 +1,25 @@
+; RUN: llc < %s | FileCheck %s
+
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @foo() local_unnamed_addr #0 {
+; CHECK-LABEL: foo:
+; CHECK:   # %bb.0:
+; CHECK-NEXT:  subq	$280, %rsp # imm = 0x118
+; CHECK-NEXT:  .cfi_def_cfa_offset 288
+; CHECK-NEXT:  movl	$1, 264(%rsp)
+; CHECK-NEXT:  movl	-128(%rsp), %eax
+; CHECK-NEXT:  addq	$280, %rsp # imm = 0x118
+; CHECK-NEXT:  .cfi_def_cfa_offset 8
+; CHECK-NEXT:  retq
+
+  %a = alloca i32, i64 100, align 16
+  %b = getelementptr inbounds i32, i32* %a, i64 98
+  store volatile i32 1, i32* %b
+  %c = load volatile i32, i32* %a
+  ret i32 %c
+}
+
+attributes #0 =  {"probe-stack"="inline-asm"}
Index: llvm/test/CodeGen/X86/stack-clash-no-free-probe.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/stack-clash-no-free-probe.ll
@@ -0,0 +1,27 @@
+; RUN: llc < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @foo(i64 %i) local_unnamed_addr #0 {
+; CHECK-LABEL: foo:
+; CHECK:   # %bb.0:
+; CHECK-NEXT:  subq	$4096, %rsp # imm = 0x1000
+; CHECK-NEXT:  movq	$0, (%rsp)
+; CHECK-NEXT:  subq	$3784, %rsp # imm = 0xEC8
+; CHECK-NEXT:  .cfi_def_cfa_offset 7888
+; CHECK-NEXT:  movl	$1, -128(%rsp,%rdi,4)
+; CHECK-NEXT:  movl	-128(%rsp), %eax
+; CHECK-NEXT:  addq	$7880, %rsp # imm = 0x1EC8
+; CHECK-NEXT:  .cfi_def_cfa_offset 8
+; CHECK-NEXT:  retq
+
+  %a = alloca i32, i32 2000, align 16
+  %b = getelementptr inbounds i32, i32* %a, i64 %i
+  store volatile i32 1, i32* %b
+  %c = load volatile i32, i32* %a
+  ret i32 %c
+}
+
+attributes #0 =  {"probe-stack"="inline-asm"}
+
Index: llvm/test/CodeGen/X86/stack-clash-medium.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/stack-clash-medium.ll
@@ -0,0 +1,30 @@
+; 

[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I added quite a bunch of inline comments again :( and some high level stuff 
below.

---

Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.

---

This adds support for clang to build the directives via the OMPIRBuilder so we 
need to add tests for that. Probably "just" running existing codegen tests for 
the directives with the OMPIRBuidlder enabled.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3005
+  llvm::BranchInst *IPBBTI =
+  llvm::dyn_cast(IPBB->getTerminator());
+  llvm::BasicBlock *DestBB = IPBBTI->getSuccessor(0);

Can you add an assertion that the branch has only a single successor.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3009
+  // erase and replace with cleanup branch.
+  IPBB->getTerminator()->eraseFromParent();
+  Builder.SetInsertPoint(IPBB);

`IPBBTI->eraseFromParent()`



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020
+  if (AllocaIP.isSet())
+AllocaInsertPt = &*AllocaIP.getPoint();
+  auto OldReturnBlock = ReturnBlock;

Why the `isSet` check? If we need it, then it's missing in other places as well 
(I think).



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3060
+? nullptr
+: Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.Int32Ty, false);
+

Can we do
```
llvm::Value *HintInst = nullptr;
if (Hint)
  ...
```
these three lines look ugly :(



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3076
+  EmitBranchThroughCleanup(Dest);
+};
+

I somehow have the feeling we could have a "FiniCB" helper function as they 
seem to be always the same. A TODO mentioning that is also OK for now.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3099
+  ReturnBlock = OldReturnBlock;
+};
+

Same here, that looks like the other "BodyGenCB". We should really make them 
helpers. 



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:259
+  /// \param BodyGenCB Callback that will generate the region code.
+  ///
+  /// \returns The insertion position *after* the master.

FiniCB param docu is missing.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:295
+ BasicBlock *ExitBB,
+ bool conditional = false);
+

`Conditional`



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:312
+Instruction *ExitCall,
+bool hasFinalize);
+

`HasFinalize`



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:331
+
+  InsertPointTy EmitOMPInlinedRegion(omp::Directive OMPD,
+ Instruction *EntryCall,

Format: no newline, above the comments got broken



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:349
+  const llvm::Twine ,
+  unsigned AddressSpace);
 };

All methods need documentation of some kind. `llvm::` is not needed here. It 
seems one method is declared twice.



Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:68
+  // Type::getInt32Ty(M.getContext()),/*NumElements*/ 8);
+  //  KmpCriticalNamePtrTy = PointerType::getUnqual(KmpCriticalNameTy);
   // Create all simple and struct types exposed by the runtime and remember

These comments that reference `KmpCriticalNameTy` do not help here. If you 
want, feel free to add comments that explain how the result looks in a generic 
way but only showing `KmpCriticalNameTy` seems counterproductive to me.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:674
+  SmallVector EnterArgs(std::begin(Args), std::end(Args));
+  Function *fn = nullptr;
+  if (HintInst) {


Spelling: `Fn`



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:695
+BodyGenCallbackTy BodyGenCB, FinalizeCallbackTy FiniCB, bool conditional,
+bool hasFinalize) {
+

Spelling `Conditional`, `HasFinalize`



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:700
+  // Create 'Critical' entry and body blocks, in preparation
+  // for conditional creation
+  BasicBlock *EntryBB = Builder.GetInsertBlock();

Outdated comment.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:717
+  // emit exit call and do any needed finalization.
+  auto FinIP = InsertPointTy(FiniBB, FiniBB->getFirstInsertionPt());
+  assert(FiniBB->getTerminator()->getNumSuccessors() == 1 &&