[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2920570 , @aeubanks wrote:

> you can reopen the revision via "Add Action..."

Hi @aeubanks , thank you for the suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

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

[PATCH] D106614: [Clang] add btf_tag attribute

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6841
+  if (auto *Rec = dyn_cast(D)) {
+if (!Rec->isCompleteDefinition()) {
+  S.Diag(AL.getLoc(), diag::warn_btftag_attribute_fwd_decl_ignored) << Str;

aaron.ballman wrote:
> Does this work for code like:
> ```
> struct __attribute__((btf_tag(""))) S {
>   int x;
> };
> ```
> (I didn't see a test case for the attribute being written in that location.)
Yes, I missed this one as typically I (and kernel developers) write the 
attribute at the end of definition. I just checked that if we put attribute 
before the struct name, when the attribute is handled/merged, it is actually an 
incomplete definition.



Comment at: clang/test/Sema/attr-btf_tag.c:31
+
+int __tag2 foo(struct t1 *arg); // expected-warning {{attribute 
'btf_tag("tag2")' ignored as not in later redeclaration or definition}}
+int __tag foo(struct t1 *arg);

aaron.ballman wrote:
> This looks backwards to me -- I think the initial declaration of `foo()` is 
> fine and shouldn't be diagnosed, it's the subsequent declarations of `foo()` 
> with a different `btf_tag` argument that should be diagnosed.
> 
> I think additional test cases for these semantics is:
> ```
> void bar();
> void __tag bar(); // (I think I understood that you want this to diagnose.)
> 
> void __tag bar();
> void bar(); // (But that you don't want this to diagnose.)
> ```
> 
There are a little complication here. Maybe you can give some advice.
We could have code like
   D1: void __tag1 __tag2 bar();
   D2: void bar();
The reason is that we explicitly allow multiple btf_tag's in the same 
declaration and this is exactly our use case.

Current merge*Attribute() framework will take one attribute at a time.
For example, first we will do
mergeBTFTagAttribute(D2, __tag1):
  Here, we add __tag1 to D2, so we have
   D2: void __tag1 bar();
we then do
mergeBTFTagAttribute(D2, __tag2):
  but D2 already has a __tag1 which is incompatible with __tag2, and
  we will issue an error here. But it is incorrect to issue an error
  since the correct semantics is to inherit both __tag1 and __tag2 for D2.

Let me take a look at the code how to best handle such cases. Please also let 
me know if you have any suggestions. thanks!

A little bit more. The following are possible cases to check:
   (1)  void bar();
 void __tag1 __tag2 bar();  // fail
   (2).  void __tag1 bar();
 void __tag1 __tag2 bar(); // fail
   (3)   void __tag1 __tag2 bar();
 void __tag1 __tag2 bar();  // succeed
   (4). void __tag1 __tag2 __tag3 bar();
 void __tag1 __tag2 bar();   // fail
   (5). void __tag3 bar();
 void __tag1 __tag2 bar(); // fail
   (6). void __tag1 __tag2 bar();
 void bar()  // succeed

Basically, for two same declaration except attributes, D1, D2,
   no error/warning only if D2 has no btf_tag attributes, or
   D1 and D2 have identical btf_tag attributes.
Do this sound reasonable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614

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

[PATCH] D105225: [clang] Add support for optional flag -fnew-infallible to restrict exception propagation

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Sorry I'm late to the party here... is there any work ongoing to add this to 
GCC too? If not, would it make sense to send a quick note to the GCC 
development list pointing this out to reduce the chance that they add a similar 
feature with a different flag name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105225

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

[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2921424 , @thakis wrote:

> Any news here? This has been broken for over a day now.

Hi @thakis, 
YES. I just create a new differential diff for it, 
https://reviews.llvm.org/D107325, could you help me to review it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

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

[PATCH] D107294: [clang-tidy] adds warning to suggest users replace symbols with words

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb updated this revision to Diff 363638.
cjdb retitled this revision from "[clang] adds warning to suggest users replace 
symbols with alt tokens" to "[clang-tidy] adds warning to suggest users replace 
symbols with words".
cjdb edited the summary of this revision.
cjdb added a comment.
Herald added subscribers: llvm-commits, xazax.hun, mgorny.
Herald added projects: LLVM, clang-tools-extra.

moves patch from Clang to clang-tidy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107294

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp
  clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-use-alternative-tokens.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn
@@ -51,6 +51,7 @@
 "SuspiciousCallArgumentCheck.cpp",
 "UniqueptrDeleteReleaseCheck.cpp",
 "UppercaseLiteralSuffixCheck.cpp",
+"UseAlternativeTokensCheck.cpp",
 "UseAnyOfAllOfCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-alternative-tokens.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-alternative-tokens.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s readability-use-alternative-tokens %t -- -- -I%S/Inputs
+
+// A note to the reader: the whitespace in this file is important: `true&&false`
+// is lexically three tokens, but `trueandfalse` is lexed as a single
+// identifier. This test needs to make sure that the fixit applies whitespace in
+// its change.
+
+// clang-format off
+
+void f1()
+{
+  (void)(true and true); // no warning
+  (void)(true&&false);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: true and false
+
+  (void)(1 bitand 2); // no warning
+  (void)(0&1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+
+  (void)(1 bitor 2); // no warning
+  (void)(0|1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+
+  (void)(compl 1); // no warning
+  (void)(~0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'compl' for bitwise negation
+  // CHECK-FIXES: compl 0
+
+  (void)(not true);
+  (void)(!false);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation
+  // CHECK-FIXES: not false
+
+  (void)(true or true);
+  (void)(true||false);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: true or false
+
+  (void)(1 xor 0);
+  (void)(0^1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: 0 xor 1
+}
+
+struct S {
+  S operator and(S) const;
+  S operator bitand(S) const;
+  S operator bitor(S) const;
+  S operator compl() const;
+  S operator not() const;
+  S operator or(S) const;
+  S operator xor(S) const;
+};
+
+void f5()
+{
+  S x;
+  S y;
+
+  (void)(x&&y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: x and y
+
+  (void)(x&y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: x bitand y
+
+  (void)(x|y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: x bitor y
+
+  (void)(~x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'compl' for bitwise negation
+  // CHECK-FIXES: compl x
+
+  (void)(!x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation
+  // CHECK-FIXES: not x
+
+  (void)(x||y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: x or y
+
+  (void)(x^y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: x xor y
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst
@@ -0,0 +1,37 @@
+.. title:: clang-tidy - readability-use-alternative-tokens
+
+readability-use-alternative-tokens
+==
+
+Finds uses of symbol-based logical and bi

[PATCH] D107267: [clang-format] don't break between function and function name in JS

[Owen Pan via Phabricator via cfe-commits]

owenpan accepted this revision.
owenpan added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107267

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

[clang] ce49fd0 - [clang] [MinGW] Let the last of -mconsole/-mwindows have effect

[Martin Storsjö via cfe-commits]

Author: Martin Storsjö
Date: 2021-08-03T10:55:44+03:00
New Revision: ce49fd024b43bd76b149f984b8f0d16e92b9bb06

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

LOG: [clang] [MinGW] Let the last of -mconsole/-mwindows have effect

Don't just check for the existence of one, but check which one was
specified last, if any.

This fixes https://llvm.org/PR51296.

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

Added: 


Modified: 
clang/lib/Driver/ToolChains/MinGW.cpp
clang/test/Driver/mingw.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/MinGW.cpp 
b/clang/lib/Driver/ToolChains/MinGW.cpp
index 20efbdc237a8..7ba729f36bd8 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -136,10 +136,13 @@ void tools::MinGW::Linker::ConstructJob(Compilation &C, 
const JobAction &JA,
 llvm_unreachable("Unsupported target architecture.");
   }
 
-  if (Args.hasArg(options::OPT_mwindows)) {
+  Arg *SubsysArg =
+  Args.getLastArg(options::OPT_mwindows, options::OPT_mconsole);
+  if (SubsysArg && SubsysArg->getOption().matches(options::OPT_mwindows)) {
 CmdArgs.push_back("--subsystem");
 CmdArgs.push_back("windows");
-  } else if (Args.hasArg(options::OPT_mconsole)) {
+  } else if (SubsysArg &&
+ SubsysArg->getOption().matches(options::OPT_mconsole)) {
 CmdArgs.push_back("--subsystem");
 CmdArgs.push_back("console");
   }

diff  --git a/clang/test/Driver/mingw.cpp b/clang/test/Driver/mingw.cpp
index 796bde8eebe4..83b61ba7d33f 100644
--- a/clang/test/Driver/mingw.cpp
+++ b/clang/test/Driver/mingw.cpp
@@ -61,3 +61,10 @@
 // RUN: %clang -target i686-windows-gnu -E -### %s -municode 2>&1 | FileCheck 
-check-prefix=CHECK_MINGW_UNICODE %s
 // CHECK_MINGW_NO_UNICODE-NOT: "-DUNICODE"
 // CHECK_MINGW_UNICODE: "-DUNICODE"
+
+// RUN: %clang -target i686-windows-gnu -### %s 2>&1 | FileCheck 
-check-prefix=CHECK_NO_SUBSYS %s
+// RUN: %clang -target i686-windows-gnu -### %s -mwindows -mconsole 2>&1 | 
FileCheck -check-prefix=CHECK_SUBSYS_CONSOLE %s
+// RUN: %clang -target i686-windows-gnu -### %s -mconsole -mwindows 2>&1 | 
FileCheck -check-prefix=CHECK_SUBSYS_WINDOWS %s
+// CHECK_NO_SUBSYS-NOT: "--subsystem"
+// CHECK_SUBSYS_CONSOLE: "--subsystem" "console"
+// CHECK_SUBSYS_WINDOWS: "--subsystem" "windows"



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

[PATCH] D107261: [clang] [MinGW] Let the last of -mconsole/-mwindows have effect

[Martin Storsjö via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGce49fd024b43: [clang] [MinGW] Let the last of 
-mconsole/-mwindows have effect (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107261

Files:
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/test/Driver/mingw.cpp


Index: clang/test/Driver/mingw.cpp
===
--- clang/test/Driver/mingw.cpp
+++ clang/test/Driver/mingw.cpp
@@ -61,3 +61,10 @@
 // RUN: %clang -target i686-windows-gnu -E -### %s -municode 2>&1 | FileCheck 
-check-prefix=CHECK_MINGW_UNICODE %s
 // CHECK_MINGW_NO_UNICODE-NOT: "-DUNICODE"
 // CHECK_MINGW_UNICODE: "-DUNICODE"
+
+// RUN: %clang -target i686-windows-gnu -### %s 2>&1 | FileCheck 
-check-prefix=CHECK_NO_SUBSYS %s
+// RUN: %clang -target i686-windows-gnu -### %s -mwindows -mconsole 2>&1 | 
FileCheck -check-prefix=CHECK_SUBSYS_CONSOLE %s
+// RUN: %clang -target i686-windows-gnu -### %s -mconsole -mwindows 2>&1 | 
FileCheck -check-prefix=CHECK_SUBSYS_WINDOWS %s
+// CHECK_NO_SUBSYS-NOT: "--subsystem"
+// CHECK_SUBSYS_CONSOLE: "--subsystem" "console"
+// CHECK_SUBSYS_WINDOWS: "--subsystem" "windows"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -136,10 +136,13 @@
 llvm_unreachable("Unsupported target architecture.");
   }
 
-  if (Args.hasArg(options::OPT_mwindows)) {
+  Arg *SubsysArg =
+  Args.getLastArg(options::OPT_mwindows, options::OPT_mconsole);
+  if (SubsysArg && SubsysArg->getOption().matches(options::OPT_mwindows)) {
 CmdArgs.push_back("--subsystem");
 CmdArgs.push_back("windows");
-  } else if (Args.hasArg(options::OPT_mconsole)) {
+  } else if (SubsysArg &&
+ SubsysArg->getOption().matches(options::OPT_mconsole)) {
 CmdArgs.push_back("--subsystem");
 CmdArgs.push_back("console");
   }


Index: clang/test/Driver/mingw.cpp
===
--- clang/test/Driver/mingw.cpp
+++ clang/test/Driver/mingw.cpp
@@ -61,3 +61,10 @@
 // RUN: %clang -target i686-windows-gnu -E -### %s -municode 2>&1 | FileCheck -check-prefix=CHECK_MINGW_UNICODE %s
 // CHECK_MINGW_NO_UNICODE-NOT: "-DUNICODE"
 // CHECK_MINGW_UNICODE: "-DUNICODE"
+
+// RUN: %clang -target i686-windows-gnu -### %s 2>&1 | FileCheck -check-prefix=CHECK_NO_SUBSYS %s
+// RUN: %clang -target i686-windows-gnu -### %s -mwindows -mconsole 2>&1 | FileCheck -check-prefix=CHECK_SUBSYS_CONSOLE %s
+// RUN: %clang -target i686-windows-gnu -### %s -mconsole -mwindows 2>&1 | FileCheck -check-prefix=CHECK_SUBSYS_WINDOWS %s
+// CHECK_NO_SUBSYS-NOT: "--subsystem"
+// CHECK_SUBSYS_CONSOLE: "--subsystem" "console"
+// CHECK_SUBSYS_WINDOWS: "--subsystem" "windows"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -136,10 +136,13 @@
 llvm_unreachable("Unsupported target architecture.");
   }
 
-  if (Args.hasArg(options::OPT_mwindows)) {
+  Arg *SubsysArg =
+  Args.getLastArg(options::OPT_mwindows, options::OPT_mconsole);
+  if (SubsysArg && SubsysArg->getOption().matches(options::OPT_mwindows)) {
 CmdArgs.push_back("--subsystem");
 CmdArgs.push_back("windows");
-  } else if (Args.hasArg(options::OPT_mconsole)) {
+  } else if (SubsysArg &&
+ SubsysArg->getOption().matches(options::OPT_mconsole)) {
 CmdArgs.push_back("--subsystem");
 CmdArgs.push_back("console");
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105003: [Analyzer] Improve report of "indeterminate file position" condition (alpha.unix.Stream).

[Balázs Kéri via Phabricator via cfe-commits]

balazske abandoned this revision.
balazske added a comment.

Adding this functionality is continued in D106262 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105003

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

[PATCH] D75045: [analyzer] Improved check of `fgetc` in StreamChecker.

[Balázs Kéri via Phabricator via cfe-commits]

balazske abandoned this revision.
balazske added a comment.
Herald added subscribers: manas, steakhal.

The idea of this change still applies but because checker and other code 
changes it is better to start again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75045

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

[PATCH] D107304: [clangd][query-driver] Extract GCC version from the driver output

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I am not sure this is a good idea. Surely we can go with this approach and 
pretend to have the same GNUC version as the toolchain, but we are still using 
clang underneath as @joerg pointed out and we might not be able to parse 
language/compiler extensions supported by GNU compilers, which will lead to 
confusion.

I think in such cases people should just have a `.clangd` file in their repo 
that adds `-fgnuc-version=x.y.z` to compile flags, i.e:

  CompileFlags:
Add: -fgnuc-version=x.y.z


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107304

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

[PATCH] D107269: [clan-format] detect function definitions more conservatively

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2485-2486
   if (Next->Next && Next->Next->is(tok::identifier) &&
-  !(Next->MatchingParen->Next && Next->MatchingParen->Next->is(tok::semi)))
+  (!Next->MatchingParen->Next ||
+   Next->MatchingParen->Next->is(tok::l_brace)))
 return true;

I was contemplating the above in https://reviews.llvm.org/D105964 and should 
have used it instead. This is probably more conservative and accurate than 
checking for an `l_brace` following the `r_paren`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107269

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

[PATCH] D107294: [clang-tidy] adds warning to suggest users replace symbols with words

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

This seems to be very old code, there was a licence change approx. 2 years ago.



Comment at: 
clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h:20-22
+// Flags uses of symbol-based bitwise and logical operators.
+
+class UseAlternativeTokensCheck : public ClangTidyCheck {

But also there is a convention for the documentation comment for the main class 
of checks, and the code here should adhere to that.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst:14
+
+This check doesn't yet account for program composition.
+

What does this refer to? Perhaps a code example and a clearly indicated 
//Limitations// heading would be more clear to indicate whether someone would 
want to enable this check.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst:21-27
+  // warning: use 'bitand' for bitwise conjunctions
+  x & y
+
+  // warning: use 'bitor' for bitwise disjunctions
+  x | y
+
+  // warning: use 'compl' for bitwise negation

Either use plural xor singular for all of these printouts, to keep consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107294

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

[PATCH] D107304: [clangd][query-driver] Extract GCC version from the driver output

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

In D107304#2921588 , @kadircet wrote:

> I am not sure this is a good idea. Surely we can go with this approach and 
> pretend to have the same GNUC version as the toolchain, but we are still 
> using clang underneath as @joerg pointed out and we might not be able to 
> parse language/compiler extensions supported by GNU compilers, which will 
> lead to confusion.

The most confusing thing for me that we already have this implicitly set 
`-fgnuc-version=4.2.1`. So, we already have implicitly set `__GNUC__` and other 
GCC macros as a default behavior.

> I think in such cases people should just have a `.clangd` file in their repo 
> that adds `-fgnuc-version=x.y.z` to compile flags, i.e:
>
>   CompileFlags:
> Add: -fgnuc-version=x.y.z
>
> This will be a more explicit step taken by the user, hence they'll be more 
> likely to know why clangd is not behaving the way they want it to.

Sometimes it's not an easy task to understand that errors in diagnostics 
returned by clangd are the result of incorrect `__GNUC__` value (in many 
projects it's not just `#error ...`). Also it's not convenient to use `.clangd` 
config in this case:

- change compiler version => update `.clangd`
- several build directories (e.g. native and cross with different GCC versions).
- idea was to make it in more automatic way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107304

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

[PATCH] D105904: [clangd] Support `#pragma mark` in the outline

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:682
+  // here since editors won't properly render the symbol otherwise.
+  StringRef MaybeGroupName = Name;
+  if (MaybeGroupName.consume_front("-") &&

dgoldman wrote:
> kadircet wrote:
> > I think this reads easier:
> > 
> > ```
> > bool IsGroup = Name.consume_front("-");
> > Name = Name.ltrim();
> > if (Name.empty())
> >   Name = IsGroup ? "unnamed group" : ...;
> > ```
> That behavior is slightly different, we want to treat `#pragma mark -Foo` as 
> `-Foo` non group but `#pragma mark - Foo` as `Foo` group.
oh I see. I indeed missed the `\s+` in the comment assumed it was `\s*`.
Is it really important to have that difference? If so, it might be useful to 
spell it out explicitly (e.g. `-Foo is not considered as a group`)



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:535
+/// by range.
+std::vector mergePragmas(std::vector &Syms,
+ std::vector 
&Pragmas,

dgoldman wrote:
> kadircet wrote:
> > dgoldman wrote:
> > > kadircet wrote:
> > > > dgoldman wrote:
> > > > > sammccall wrote:
> > > > > > FWIW the flow control/how we make progress seem hard to follow here 
> > > > > > to me.
> > > > > > 
> > > > > > In particular I think I'm struggling with the statefulness of "is 
> > > > > > there an open mark group".
> > > > > > 
> > > > > > Possible simplifications:
> > > > > >  - define a dummy root symbol, which seems clearer than the 
> > > > > > vector + range
> > > > > >  - avoid reverse-sorting the list of pragma symbols, and just 
> > > > > > consume from the front of an ArrayRef instead
> > > > > >  - make the outer loop over pragmas, rather than symbols. It would 
> > > > > > first check if the pragma belongs directly here or not, and if so, 
> > > > > > loop over symbols to work out which should become children. This 
> > > > > > seems very likely to be efficient enough in practice (few pragmas, 
> > > > > > or most children are grouped into pragmas)
> > > > > > define a dummy root symbol, which seems clearer than the 
> > > > > > vector + range
> > > > > 
> > > > > I guess? Then we'd take in a `DocumentSymbol & and a 
> > > > > ArrayRef & (or just by value and then return it as 
> > > > > well). The rest would be the same though
> > > > > 
> > > > > > In particular I think I'm struggling with the statefulness of "is 
> > > > > > there an open mark group".
> > > > > 
> > > > > We need to track the current open group if there is one in order to 
> > > > > move children to it.
> > > > > 
> > > > > > make the outer loop over pragmas, rather than symbols. It would 
> > > > > > first check if the pragma belongs directly here or not, and if so, 
> > > > > > loop over symbols to work out which should become children. This 
> > > > > > seems very likely to be efficient enough in practice (few pragmas, 
> > > > > > or most children are grouped into pragmas)
> > > > > 
> > > > > The important thing here is knowing where the pragma mark ends - if 
> > > > > it doesn't, it actually gets all of the children. So we'd have to 
> > > > > peak at the next pragma mark, add all symbols before it to us as 
> > > > > children, and then potentially recurse to nest it inside of a symbol. 
> > > > > I'll try it out and see if it's simpler.
> > > > > 
> > > > > 
> > > > ```
> > > > while(Pragmas) {
> > > > // We'll figure out where the Pragmas.front() should go.
> > > > Pragma P = Pragmas.front();
> > > > DocumentSymbol *Cur = Root;
> > > > while(Cur->contains(P)) {
> > > >   auto *OldCur = Cur;
> > > >   for(auto *C : Cur->children) {
> > > >  // We assume at most 1 child can contain the pragma (as pragmas 
> > > > are on a single line, and children have disjoint ranges)
> > > >  if (C->contains(P)) {
> > > >  Cur = C;
> > > >  break;
> > > >  }
> > > >   }
> > > >   // Cur is immediate parent of P
> > > >   if (OldCur == Cur) {
> > > > // Just insert P into children if it is not a group and we are done.
> > > > // Otherwise we need to figure out when current pragma is 
> > > > terminated:
> > > > // if next pragma is not contained in Cur, or is contained in one of 
> > > > the children, It is at the end of Cur, nest all the children that 
> > > > appear after P under the symbol node for P.
> > > > // Otherwise nest all the children that appear after P but before next 
> > > > pragma under the symbol node for P.
> > > > // Pop Pragmas and break
> > > >   }
> > > > }
> > > > }
> > > > ```
> > > > 
> > > > Does that make sense, i hope i am not missing something obvious? 
> > > > Complexity-wise in the worst case we'll go all the way down to a leaf 
> > > > once per pragma, since there will only be a handful of pragmas most of 
> > > > the time it shouldn't be too bad.
> > > I've implemented your suggestion. I don't think it's simpler, but LMK, 
> > > maybe it can be improved.
> > oops, i was loo

[PATCH] D107304: [clangd][query-driver] Extract GCC version from the driver output

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> The most confusing thing for me that we already have this implicitly set 
> -fgnuc-version=4.2.1. So, we already have implicitly set __GNUC__ and other 
> GCC macros as a default behavior.

It might be because clang actually supports all the GNU extensions up until 
4.2.1, I don't know the history here, but if clang can compile with that GNUC 
macro by default, clangd should be good as well. But there are no promises for 
anything higher than that.

> Sometimes it's not an easy task to understand that errors in diagnostics 
> returned by clangd are the result of incorrect __GNUC__ value (in many 
> projects it's not just #error ...). Also it's not convenient to use .clangd 
> config in this case:
> change compiler version => update .clangd
> several build directories (e.g. native and cross with different GCC versions).
> query-driver supports different compilers in the same compile_commands.json
> idea was to make it in more automatic way.

I think these are the reasons why this needs to be set explicitly by the user. 
Because even though it can make things work, it might as well break things 
implicitly (e.g. you have a compiler version 4.2.1 everything works as 
expected, but once you update your compiler to gcc-5 clangd breaks all of a 
sudden)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107304

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

[clang] 4f4f278 - [clang-format] don't break between function and function name in JS

[Krasimir Georgiev via cfe-commits]

Author: Krasimir Georgiev
Date: 2021-08-03T11:18:21+02:00
New Revision: 4f4f2783056fd01182740251b2ce8a77b12684b3

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

LOG: [clang-format] don't break between function and function name in JS

The patch https://reviews.llvm.org/D105964 
(https://github.com/llvm/llvm-project/commit/58494c856a15f5b0e886c7baf5d505ac6c05dfe5)
updated detection of function declaration names. It had the unfortunate
consequence that it started breaking between `function` and the function
name in some cases in JavaScript code.

This patch addresses this.

Reviewed By: MyDeveloperDay, owenpan

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

Added: 


Modified: 
clang/lib/Format/ContinuationIndenter.cpp
clang/unittests/Format/FormatTestJS.cpp

Removed: 




diff  --git a/clang/lib/Format/ContinuationIndenter.cpp 
b/clang/lib/Format/ContinuationIndenter.cpp
index 8fbc15f27922a..cdb112726b80d 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -495,7 +495,10 @@ bool ContinuationIndenter::mustBreak(const LineState 
&State) {
   if (((Current.is(TT_FunctionDeclarationName) &&
 // Don't break before a C# function when no break after return type
 (!Style.isCSharp() ||
- Style.AlwaysBreakAfterReturnType != FormatStyle::RTBS_None)) ||
+ Style.AlwaysBreakAfterReturnType != FormatStyle::RTBS_None) &&
+// Don't always break between a JavaScript `function` and the function
+// name.
+Style.Language != FormatStyle::LK_JavaScript) ||
(Current.is(tok::kw_operator) && !Previous.is(tok::coloncolon))) &&
   !Previous.is(tok::kw_template) && 
State.Stack.back().BreakBeforeParameter)
 return true;

diff  --git a/clang/unittests/Format/FormatTestJS.cpp 
b/clang/unittests/Format/FormatTestJS.cpp
index c92c81f66a870..1c174e9e5b170 100644
--- a/clang/unittests/Format/FormatTestJS.cpp
+++ b/clang/unittests/Format/FormatTestJS.cpp
@@ -692,6 +692,13 @@ TEST_F(FormatTestJS, FormatsFreestandingFunctions) {
"  let x = 1;\n"
"  console.log(x);\n"
"}\n");
+  EXPECT_EQ("a = function(x) {}\n"
+"\n"
+"function f(x) {}",
+format("a = function(x) {}\n"
+   "\n"
+   "function f(x) {}",
+   getGoogleJSStyleWithColumns(20)));
 }
 
 TEST_F(FormatTestJS, GeneratorFunctions) {



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

[PATCH] D107267: [clang-format] don't break between function and function name in JS

[Krasimir Georgiev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4f4f2783056f: [clang-format] don't break between 
function and function name in JS (authored by krasimir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107267

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTestJS.cpp


Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -692,6 +692,13 @@
"  let x = 1;\n"
"  console.log(x);\n"
"}\n");
+  EXPECT_EQ("a = function(x) {}\n"
+"\n"
+"function f(x) {}",
+format("a = function(x) {}\n"
+   "\n"
+   "function f(x) {}",
+   getGoogleJSStyleWithColumns(20)));
 }
 
 TEST_F(FormatTestJS, GeneratorFunctions) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -495,7 +495,10 @@
   if (((Current.is(TT_FunctionDeclarationName) &&
 // Don't break before a C# function when no break after return type
 (!Style.isCSharp() ||
- Style.AlwaysBreakAfterReturnType != FormatStyle::RTBS_None)) ||
+ Style.AlwaysBreakAfterReturnType != FormatStyle::RTBS_None) &&
+// Don't always break between a JavaScript `function` and the function
+// name.
+Style.Language != FormatStyle::LK_JavaScript) ||
(Current.is(tok::kw_operator) && !Previous.is(tok::coloncolon))) &&
   !Previous.is(tok::kw_template) && 
State.Stack.back().BreakBeforeParameter)
 return true;


Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -692,6 +692,13 @@
"  let x = 1;\n"
"  console.log(x);\n"
"}\n");
+  EXPECT_EQ("a = function(x) {}\n"
+"\n"
+"function f(x) {}",
+format("a = function(x) {}\n"
+   "\n"
+   "function f(x) {}",
+   getGoogleJSStyleWithColumns(20)));
 }
 
 TEST_F(FormatTestJS, GeneratorFunctions) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -495,7 +495,10 @@
   if (((Current.is(TT_FunctionDeclarationName) &&
 // Don't break before a C# function when no break after return type
 (!Style.isCSharp() ||
- Style.AlwaysBreakAfterReturnType != FormatStyle::RTBS_None)) ||
+ Style.AlwaysBreakAfterReturnType != FormatStyle::RTBS_None) &&
+// Don't always break between a JavaScript `function` and the function
+// name.
+Style.Language != FormatStyle::LK_JavaScript) ||
(Current.is(tok::kw_operator) && !Previous.is(tok::coloncolon))) &&
   !Previous.is(tok::kw_template) && State.Stack.back().BreakBeforeParameter)
 return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91950: [clang-format] Add BreakBeforeInlineASMColon configuration

[Anastasiia Lukianenko via Phabricator via cfe-commits]

anastasiia_lukianenko added inline comments.



Comment at: clang/lib/Format/Format.cpp:1022
   LLVMStyle.BreakBeforeConceptDeclarations = true;
+  LLVMStyle.BreakBeforeInlineASMColon = FormatStyle::BBIAS_OnlyMultiline;
   LLVMStyle.BreakBeforeTernaryOperators = true;

MyDeveloperDay wrote:
> Are we happy that is the correct default? didn't it always break before?
It didn't always break before. Old tests in FormatsInlineASM block prove this:


```
verifyFormat("asm(\"nop\" ::: \"memory\");");
  verifyFormat(
  "asm(\"movq\\t%%rbx, %%rsi\\n\\t\"\n"
  "\"cpuid\\n\\t\"\n"
  "\"xchgq\\t%%rbx, %%rsi\\n\\t\"\n"
  ": \"=a\"(*rEAX), \"=S\"(*rEBX), \"=c\"(*rECX), \"=d\"(*rEDX)\n"
  ": \"a\"(value));");
```


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

https://reviews.llvm.org/D91950

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

[PATCH] D107318: [OpenCL] allow generic address and non-generic defs for CL3.0

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

Yeah, this looks good. Thanks! Please notify when this is ready to land.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107318

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

[PATCH] D107269: [clan-format] detect function definitions more conservatively

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir updated this revision to Diff 363669.
krasimir added a comment.

- apply review suggestion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107269

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8224,7 +8224,12 @@
"f(i)\n"
"{\n"
"  return i + 1;\n"
-   "}",
+   "}\n"
+   "int\n" // Break here.
+   "f(i)\n"
+   "{\n"
+   "  return i + 1;\n"
+   "};",
Style);
   verifyFormat("int f(a, b, c);\n" // No break here.
"int\n" // Break here.
@@ -8233,8 +8238,20 @@
"float c;\n"
"{\n"
"  return a + b < c;\n"
-   "}",
+   "}\n"
+   "int\n"// Break here.
+   "f(a, b, c)\n" // Break here.
+   "short a, b;\n"
+   "float c;\n"
+   "{\n"
+   "  return a + b < c;\n"
+   "};",
Style);
+  // The return breaking style doesn't affect object definitions with
+  // attribute-like macros.
+  verifyFormat("Tttt ppp\n"
+   "ABSL_GUARDED_BY(mutex) = {};",
+   getGoogleStyleWithColumns(40));
 
   Style = getGNUStyle();
 
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2482,7 +2482,7 @@
   // return i + 1;
   //   }
   if (Next->Next && Next->Next->is(tok::identifier) &&
-  !(Next->MatchingParen->Next && Next->MatchingParen->Next->is(tok::semi)))
+  Line.Last->isNot(tok::semi))
 return true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8224,7 +8224,12 @@
"f(i)\n"
"{\n"
"  return i + 1;\n"
-   "}",
+   "}\n"
+   "int\n" // Break here.
+   "f(i)\n"
+   "{\n"
+   "  return i + 1;\n"
+   "};",
Style);
   verifyFormat("int f(a, b, c);\n" // No break here.
"int\n" // Break here.
@@ -8233,8 +8238,20 @@
"float c;\n"
"{\n"
"  return a + b < c;\n"
-   "}",
+   "}\n"
+   "int\n"// Break here.
+   "f(a, b, c)\n" // Break here.
+   "short a, b;\n"
+   "float c;\n"
+   "{\n"
+   "  return a + b < c;\n"
+   "};",
Style);
+  // The return breaking style doesn't affect object definitions with
+  // attribute-like macros.
+  verifyFormat("Tttt ppp\n"
+   "ABSL_GUARDED_BY(mutex) = {};",
+   getGoogleStyleWithColumns(40));
 
   Style = getGNUStyle();
 
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2482,7 +2482,7 @@
   // return i + 1;
   //   }
   if (Next->Next && Next->Next->is(tok::identifier) &&
-  !(Next->MatchingParen->Next && Next->MatchingParen->Next->is(tok::semi)))
+  Line.Last->isNot(tok::semi))
 return true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107269: [clan-format] detect function definitions more conservatively

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2485-2486
   if (Next->Next && Next->Next->is(tok::identifier) &&
-  !(Next->MatchingParen->Next && Next->MatchingParen->Next->is(tok::semi)))
+  (!Next->MatchingParen->Next ||
+   Next->MatchingParen->Next->is(tok::l_brace)))
 return true;

owenpan wrote:
> I was contemplating the above in https://reviews.llvm.org/D105964 and should 
> have used it instead. This is probably more conservative and accurate than 
> checking for an `l_brace` following the `r_paren`.
Thank you! 

Applied the suggestion. I was a bit worried that this suggestion won't work for 
function definitions with a trailing semicolon, but there the semicolon is not 
part of the same line as the function name, so all is good there. Added a few 
tests for this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107269

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

[PATCH] D107145: clangd: Add new semantic token modifier "virtual"

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler added a comment.

Can someone please merge this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107145

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

[clang] 08128fe - [clang] Make member var invalid when static initializer is invalid.

[Adam Czachorowski via cfe-commits]

Author: Adam Czachorowski
Date: 2021-08-03T11:52:52+02:00
New Revision: 08128fe7059e20b3f97ae5abbdeff2e6f6c711ed

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

LOG: [clang] Make member var invalid when static initializer is invalid.

Previously we would show an error, but keep the member, and also the
CXXRrecordDecl, valid. This could lead to crashes when attempting to
access the record layout or size.

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

Added: 
clang/test/AST/ast-dump-undeduced-expr.cpp

Modified: 
clang/lib/Parse/ParseDeclCXX.cpp
clang/test/SemaCXX/crash-auto-36064.cpp
clang/test/SemaCXX/cxx11-crashes.cpp

Removed: 




diff  --git a/clang/lib/Parse/ParseDeclCXX.cpp 
b/clang/lib/Parse/ParseDeclCXX.cpp
index ca5c013a51fe0..760600a3ea3ca 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -2989,9 +2989,11 @@ Parser::ParseCXXClassMemberDeclaration(AccessSpecifier 
AS,
   ExprResult Init = ParseCXXMemberInitializer(
   ThisDecl, DeclaratorInfo.isDeclarationOfFunction(), EqualLoc);
 
-  if (Init.isInvalid())
+  if (Init.isInvalid()) {
+if (ThisDecl)
+  Actions.ActOnUninitializedDecl(ThisDecl);
 SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
-  else if (ThisDecl)
+  } else if (ThisDecl)
 Actions.AddInitializerToDecl(ThisDecl, Init.get(), 
EqualLoc.isInvalid());
 } else if (ThisDecl && DS.getStorageClassSpec() == DeclSpec::SCS_static)
   // No initializer.

diff  --git a/clang/test/AST/ast-dump-undeduced-expr.cpp 
b/clang/test/AST/ast-dump-undeduced-expr.cpp
new file mode 100644
index 0..d404db9285974
--- /dev/null
+++ b/clang/test/AST/ast-dump-undeduced-expr.cpp
@@ -0,0 +1,7 @@
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck 
%s
+
+struct Foo {
+  static constexpr auto Bar = ;
+};
+
+// CHECK: -VarDecl {{.*}} invalid Bar 'const auto' static constexpr

diff  --git a/clang/test/SemaCXX/crash-auto-36064.cpp 
b/clang/test/SemaCXX/crash-auto-36064.cpp
index 5678cd8b730b8..a34e5eea51cb0 100644
--- a/clang/test/SemaCXX/crash-auto-36064.cpp
+++ b/clang/test/SemaCXX/crash-auto-36064.cpp
@@ -1,8 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
-template  // expected-error{{new expression for 
type 'auto' requires a constructor argument}}
+template 
 struct b;
 struct d {
   static auto c = ;  // expected-error{{expected expression}}
+ // expected-error@-1 {{declaration of 
variable 'c' with deduced type 'auto' requires an initializer}}
+
   decltype(b); // expected-error{{expected '(' for 
function-style cast or type construction}}
- // expected-note@-1{{while substituting prior 
template arguments into non-type template parameter [with A = auto]}}
 };

diff  --git a/clang/test/SemaCXX/cxx11-crashes.cpp 
b/clang/test/SemaCXX/cxx11-crashes.cpp
index d60782df35f45..e63a9be956942 100644
--- a/clang/test/SemaCXX/cxx11-crashes.cpp
+++ b/clang/test/SemaCXX/cxx11-crashes.cpp
@@ -104,3 +104,22 @@ namespace pr29091 {
   bool baz() { return __has_nothrow_constructor(B); }
   bool qux() { return __has_nothrow_copy(B); }
 }
+
+namespace undeduced_field {
+template
+struct Foo {
+  typedef T type;
+};
+
+struct Bar {
+  Bar();
+  // The missing expression makes A undeduced.
+  static constexpr auto A = ;  // expected-error {{expected expression}}
+   // expected-error@-1 {{declaration of variable 
'A' with deduced type 'const auto' requires an initializer}}
+
+  Foo::type B;  // The type of B is also undeduced (wrapped in 
Elaborated).
+};
+
+// This used to crash when trying to get the layout of B.
+Bar x;
+}



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

[PATCH] D105478: [clang] Make CXXRecrdDecl invalid if it contains any undeduced fields.

[Adam Czachorowski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG08128fe7059e: [clang] Make member var invalid when static 
initializer is invalid. (authored by adamcz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105478

Files:
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/AST/ast-dump-undeduced-expr.cpp
  clang/test/SemaCXX/crash-auto-36064.cpp
  clang/test/SemaCXX/cxx11-crashes.cpp


Index: clang/test/SemaCXX/cxx11-crashes.cpp
===
--- clang/test/SemaCXX/cxx11-crashes.cpp
+++ clang/test/SemaCXX/cxx11-crashes.cpp
@@ -104,3 +104,22 @@
   bool baz() { return __has_nothrow_constructor(B); }
   bool qux() { return __has_nothrow_copy(B); }
 }
+
+namespace undeduced_field {
+template
+struct Foo {
+  typedef T type;
+};
+
+struct Bar {
+  Bar();
+  // The missing expression makes A undeduced.
+  static constexpr auto A = ;  // expected-error {{expected expression}}
+   // expected-error@-1 {{declaration of variable 
'A' with deduced type 'const auto' requires an initializer}}
+
+  Foo::type B;  // The type of B is also undeduced (wrapped in 
Elaborated).
+};
+
+// This used to crash when trying to get the layout of B.
+Bar x;
+}
Index: clang/test/SemaCXX/crash-auto-36064.cpp
===
--- clang/test/SemaCXX/crash-auto-36064.cpp
+++ clang/test/SemaCXX/crash-auto-36064.cpp
@@ -1,8 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
-template  // expected-error{{new expression for 
type 'auto' requires a constructor argument}}
+template 
 struct b;
 struct d {
   static auto c = ;  // expected-error{{expected expression}}
+ // expected-error@-1 {{declaration of 
variable 'c' with deduced type 'auto' requires an initializer}}
+
   decltype(b); // expected-error{{expected '(' for 
function-style cast or type construction}}
- // expected-note@-1{{while substituting prior 
template arguments into non-type template parameter [with A = auto]}}
 };
Index: clang/test/AST/ast-dump-undeduced-expr.cpp
===
--- /dev/null
+++ clang/test/AST/ast-dump-undeduced-expr.cpp
@@ -0,0 +1,7 @@
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck 
%s
+
+struct Foo {
+  static constexpr auto Bar = ;
+};
+
+// CHECK: -VarDecl {{.*}} invalid Bar 'const auto' static constexpr
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2989,9 +2989,11 @@
   ExprResult Init = ParseCXXMemberInitializer(
   ThisDecl, DeclaratorInfo.isDeclarationOfFunction(), EqualLoc);
 
-  if (Init.isInvalid())
+  if (Init.isInvalid()) {
+if (ThisDecl)
+  Actions.ActOnUninitializedDecl(ThisDecl);
 SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
-  else if (ThisDecl)
+  } else if (ThisDecl)
 Actions.AddInitializerToDecl(ThisDecl, Init.get(), 
EqualLoc.isInvalid());
 } else if (ThisDecl && DS.getStorageClassSpec() == DeclSpec::SCS_static)
   // No initializer.


Index: clang/test/SemaCXX/cxx11-crashes.cpp
===
--- clang/test/SemaCXX/cxx11-crashes.cpp
+++ clang/test/SemaCXX/cxx11-crashes.cpp
@@ -104,3 +104,22 @@
   bool baz() { return __has_nothrow_constructor(B); }
   bool qux() { return __has_nothrow_copy(B); }
 }
+
+namespace undeduced_field {
+template
+struct Foo {
+  typedef T type;
+};
+
+struct Bar {
+  Bar();
+  // The missing expression makes A undeduced.
+  static constexpr auto A = ;  // expected-error {{expected expression}}
+   // expected-error@-1 {{declaration of variable 'A' with deduced type 'const auto' requires an initializer}}
+
+  Foo::type B;  // The type of B is also undeduced (wrapped in Elaborated).
+};
+
+// This used to crash when trying to get the layout of B.
+Bar x;
+}
Index: clang/test/SemaCXX/crash-auto-36064.cpp
===
--- clang/test/SemaCXX/crash-auto-36064.cpp
+++ clang/test/SemaCXX/crash-auto-36064.cpp
@@ -1,8 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
-template  // expected-error{{new expression for type 'auto' requires a constructor argument}}
+template 
 struct b;
 struct d {
   static auto c = ;  // expected-error{{expected expression}}
+ // expected-error@-1 {{declaration of variable 'c' with deduced type 'auto' requires an initializer}}
+
   decltype(b); // expected-error{{expected '(' for function-style cast or type construction}}
- // expecte

[PATCH] D107304: [clangd][query-driver] Extract GCC version from the driver output

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

In D107304#2921694 , @kadircet wrote:

>> The most confusing thing for me that we already have this implicitly set 
>> -fgnuc-version=4.2.1. So, we already have implicitly set __GNUC__ and other 
>> GCC macros as a default behavior.
>
> It might be because clang actually supports all the GNU extensions up until 
> 4.2.1, I don't know the history here, but if clang can compile with that GNUC 
> macro by default, clangd should be good as well. But there are no promises 
> for anything higher than that.

Seem the background is that it was unable to build chromium with clang: 
https://reviews.llvm.org/D68055
And as I can see there: `this flag does not enable or disable any GCC 
extensions implemented in Clang`. Thus, it just defines some GCC macros. So, 
without this patch we guaranty to user that these macros values will be 
incompatible with his GCC version if it is not 4.2.1, which is not an expected 
behavior for `--query-driver` for me: I expect that it can extract as much 
information from the driver output as possible and help clang with files 
processing.

> I think these are the reasons why this needs to be set explicitly by the 
> user. Because even though it can make things work, it might as well break 
> things implicitly (e.g. you have a compiler version 4.2.1 everything works as 
> expected, but once you update your compiler to gcc-5 clangd breaks all of a 
> sudden)

Can't agree that we have equal probabilities to break something in a project 
which was build with GCC x.y.z when specifying the correct version (x.y.z)  and 
incorrect (4.2.1).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107304

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

[PATCH] D106152: [analyzer] Move test case to existing test file and remove duplicated test file.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ just a ping


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

https://reviews.llvm.org/D106152

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

[PATCH] D107073: [analyzer] Disable direct binding from list initialization for constant arrays of local storage duration.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 363677.
ASDenysPetrov added a comment.

Refined code a bit.


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

https://reviews.llvm.org/D107073

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/initialization.c
  clang/test/Analysis/initialization.cpp

Index: clang/test/Analysis/initialization.cpp
===
--- clang/test/Analysis/initialization.cpp
+++ clang/test/Analysis/initialization.cpp
@@ -167,7 +167,7 @@
   clang_analyzer_eval(glob_arr3[3][1] == 0); // expected-warning{{TRUE}}
 }
 
-void negative_index1() {
+void glob_arr_negative_index1() {
   int x = 2, y = -2;
   clang_analyzer_eval(glob_arr3[x][y] == 3); // expected-warning{{TRUE}}
   x = 4;
@@ -175,12 +175,12 @@
   clang_analyzer_eval(glob_arr3[x][y] == 7); // expected-warning{{TRUE}}
 }
 
-void out_of_bound_index1() {
+void glob_arr_out_of_bound_index1() {
   int x = -3, y = 2;
   int res = glob_arr3[x][y]; // expected-warning{{garbage or undefined}}
 }
 
-void out_of_bound_index2() {
+void glob_arr_out_of_bound_index2() {
   int x = 3, y = 2;
   int res = glob_arr3[x][y]; // expected-warning{{garbage or undefined}}
 }
@@ -197,12 +197,101 @@
   clang_analyzer_eval(glob_arr4[7] == 0); // expected-warning{{TRUE}}
 }
 
-void out_of_bound_index3() {
+void glob_arr_out_of_bound_index3() {
   int x = -42;
   int res = glob_arr4[x]; // expected-warning{{garbage or undefined}}
 }
 
-void out_of_bound_index4() {
+void glob_arr_out_of_bound_index4() {
   int x = 42;
   int res = glob_arr4[x]; // expected-warning{{garbage or undefined}}
 }
+
+void local_arr_index1() {
+  const int local_arr[2][2][3] = {{{1, 2}}, {{7}}};
+  clang_analyzer_eval(local_arr[0][0][0] == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][0][1] == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][0][2] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][1][0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][1][1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][1][2] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][0][0] == 7); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][0][1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][0][2] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][1][0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][1][1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][1][2] == 0); // expected-warning{{TRUE}}
+}
+
+void local_arr_index2() {
+  int const local_arr[2][2][3] = {{{1, 2}, {}}, {{7, 8}, {10, 11, 12}}};
+  clang_analyzer_eval(local_arr[0][0][0] == 1);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][0][1] == 2);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][0][2] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][1][0] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][1][1] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][1][2] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][0][0] == 7);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][0][1] == 8);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][0][2] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][1][0] == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][1][1] == 11); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][1][2] == 12); // expected-warning{{TRUE}}
+}
+
+void local_arr_index3() {
+  const int local_arr[4][2] = {{}, {3}, {}, {7}};
+  clang_analyzer_eval(local_arr[0][0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[0][1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][0] == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[1][1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[2][0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[2][1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[3][0] == 7); // expected-warning{{TRUE}}
+  clang_analyzer_eval(local_arr[3][1] == 0); // expected-warning{{TRUE}}
+}
+
+void local_arr_negative_index1() {
+  const int local_arr[4][2] = {{}, {3}, {}, {7}};
+  int x = 2, y = -2;
+  clang_analyzer_eval(local_arr[x][y] == 3); // expected-warning{{TRUE}}
+  x = 4;
+  y = -2;
+  clang_analyzer_eval(local_arr[x][y] == 7); // expected-warning{{TRUE}}
+}
+
+void local_arr_out_of_bound_index1() {
+  const int local_arr[4][2] = {{}, {3}, {}, {7}};
+  int x = -3, y = 2;
+  int res = local_arr[x][y]; // expected-warning{{garbage or undefined}}
+}
+
+void local_arr_out_of_bound_index2() {
+  const int local_arr[4][2] = {{}, {3}, {}, {7}};
+  int x = 3, y = 2;
+  int res = local_

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/test/CodeGenOpenCL/fp-atomics-optremarks-gfx90a.cl:25
+// GFX90A-HW-LABEL: test_atomic_add
+// GFX90A-HW:   global_atomic_add_f64
+float test_atomic_add(global atomic_double *d, double a) {

Should check the operands etc, this isn't enough to see which version was 
selected



Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:585
+  bool UnsafeFPAtomicFlag = RMW->getFunction()
+->getFnAttribute("amdgpu-unsafe-fp-atomics")
+.getValueAsBool();

Cannot check the AMDGPU attribute here



Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:590
+  OptimizationRemark Remark(DEBUG_TYPE, "Passed", RMW->getFunction());
+  Remark << "An atomic instruction was expanded into a CAS "
+"loop.";

Maybe CAS should be spelled out



Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:591
+  Remark << "An atomic instruction was expanded into a CAS "
+"loop.";
+  return Remark;

I think remark messages aren't supposed to end in punctuation



Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:598
+  OptimizationRemark Remark(DEBUG_TYPE, "Passed", RMW->getFunction());
+  Remark << "An unsafe hardware instruction was generated.";
+  return Remark;

Unsafe is misleading, plus this is being too specific to AMDGPU


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

Should still have an IR only remark test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106909: [clang] Add clang builtins support for gfx90a

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec requested changes to this revision.
rampitec added a comment.
This revision now requires changes to proceed.

Needs an IR test, a test for different supported targets, and a negative test 
for unsupported features.




Comment at: clang/include/clang/Basic/BuiltinsAMDGPU.def:199
 
+TARGET_BUILTIN(__builtin_amdgcn_global_atomic_fadd_f64, "dd*1di", "t", 
"gfx90a-insts")
+TARGET_BUILTIN(__builtin_amdgcn_global_atomic_fadd_f32, "ff*1fi", "t", 
"gfx90a-insts")

Correct attribute for this one in atomic-fadd-insts. In particular it was first 
added in gfx908 and you would need to test it too.



Comment at: clang/include/clang/Basic/BuiltinsAMDGPU.def:205
+
+TARGET_BUILTIN(__builtin_amdgcn_flat_atomic_fadd_f64, "dd*1di", "t", 
"gfx90a-insts")
+TARGET_BUILTIN(__builtin_amdgcn_flat_atomic_fmin_f64, "dd*1di", "t", 
"gfx90a-insts")

Flat address space is 0.



Comment at: clang/include/clang/Basic/BuiltinsAMDGPU.def:210
+TARGET_BUILTIN(__builtin_amdgcn_ds_atomic_fadd_f64, "dd*3di", "t", 
"gfx90a-insts")
+TARGET_BUILTIN(__builtin_amdgcn_ds_atomic_fadd_f32, "ff*3fi", "t", 
"gfx90a-insts")
+

This is available since gfx8. Attribute gfx8-insts.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16212
+  case AMDGPU::BI__builtin_amdgcn_flat_atomic_fmax_f64: {
+Intrinsic::ID IID;
+llvm::Type *ArgTy = llvm::Type::getDoubleTy(getLLVMContext());

You do not need any of that code. You can directly map a builtin to intrinsic 
in the IntrinsicsAMDGPU.td.



Comment at: clang/test/CodeGenOpenCL/builtins-fp-atomics.cl:112
+kernel void test_flat_global_max(__global double *addr, double x){
+  __builtin_amdgcn_flat_atomic_fmax_f64(addr, x, memory_order_relaxed);
+}

arsenm wrote:
> gandhi21299 wrote:
> > arsenm wrote:
> > > If you're going to bother testing the ISA, is it worth testing rtn and no 
> > > rtn versions?
> > Sorry, what do you mean by rtn version?
> Most atomics can be optimized if they don't return the in memory value if the 
> value is unused
Certainly yes, because global_atomic_add_f32 did not have return version on 
gfx908.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106909

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec requested changes to this revision.
rampitec added a comment.
This revision now requires changes to proceed.

You cannot do it in a generic llvm code, it simply has no knowledge of what was 
the reason for BE's choice.




Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:598
+  OptimizationRemark Remark(DEBUG_TYPE, "Passed", RMW->getFunction());
+  Remark << "An unsafe hardware instruction was generated.";
+  return Remark;

arsenm wrote:
> Unsafe is misleading, plus this is being too specific to AMDGPU
Having UnsafeFPAtomicFlag does not automatically mean a HW instruction produced 
is unsafe. Moreover, you simply cannot know why this or that decision was done 
by a target method here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Anshil Gandhi via Phabricator via cfe-commits]

gandhi21299 updated this revision to Diff 363590.
gandhi21299 marked an inline comment as done.
gandhi21299 added a comment.

requested changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

Files:
  clang/test/CodeGenCUDA/fp-atomics-optremarks.cu
  clang/test/CodeGenOpenCL/fp-atomics-optremarks-gfx90a.cl
  llvm/lib/CodeGen/AtomicExpandPass.cpp
  llvm/test/CodeGen/AMDGPU/fp-atomics-remarks-gfx90a.ll
  llvm/test/CodeGen/AMDGPU/llc-pipeline.ll
  llvm/test/CodeGen/X86/O0-pipeline.ll
  llvm/test/CodeGen/X86/opt-pipeline.ll

Index: llvm/test/CodeGen/X86/opt-pipeline.ll
===
--- llvm/test/CodeGen/X86/opt-pipeline.ll
+++ llvm/test/CodeGen/X86/opt-pipeline.ll
@@ -16,15 +16,20 @@
 ; CHECK-NEXT: Target Pass Configuration
 ; CHECK-NEXT: Machine Module Information
 ; CHECK-NEXT: Target Transform Information
+; CHECK-NEXT: Profile summary info
 ; CHECK-NEXT: Type-Based Alias Analysis
 ; CHECK-NEXT: Scoped NoAlias Alias Analysis
 ; CHECK-NEXT: Assumption Cache Tracker
-; CHECK-NEXT: Profile summary info
 ; CHECK-NEXT: Create Garbage Collector Module Metadata
 ; CHECK-NEXT: Machine Branch Probability Analysis
 ; CHECK-NEXT:   ModulePass Manager
 ; CHECK-NEXT: Pre-ISel Intrinsic Lowering
 ; CHECK-NEXT: FunctionPass Manager
+; CHECK-NEXT:   Dominator Tree Construction 
+; CHECK-NEXT:   Natural Loop Information 
+; CHECK-NEXT:   Lazy Branch Probability Analysis 
+; CHECK-NEXT:   Lazy Block Frequency Analysis 
+; CHECK-NEXT:   Optimization Remark Emitter
 ; CHECK-NEXT:   Expand Atomic instructions
 ; CHECK-NEXT:   Lower AMX intrinsics
 ; CHECK-NEXT:   Lower AMX type for load/store
Index: llvm/test/CodeGen/X86/O0-pipeline.ll
===
--- llvm/test/CodeGen/X86/O0-pipeline.ll
+++ llvm/test/CodeGen/X86/O0-pipeline.ll
@@ -10,13 +10,18 @@
 ; CHECK-NEXT: Target Pass Configuration
 ; CHECK-NEXT: Machine Module Information
 ; CHECK-NEXT: Target Transform Information
+; CHECK-NEXT: Profile summary info
 ; CHECK-NEXT: Create Garbage Collector Module Metadata
 ; CHECK-NEXT: Assumption Cache Tracker
-; CHECK-NEXT: Profile summary info
 ; CHECK-NEXT: Machine Branch Probability Analysis
 ; CHECK-NEXT:   ModulePass Manager
 ; CHECK-NEXT: Pre-ISel Intrinsic Lowering
 ; CHECK-NEXT: FunctionPass Manager
+; CHECK-NEXT:   Dominator Tree Construction 
+; CHECK-NEXT:   Natural Loop Information 
+; CHECK-NEXT:   Lazy Branch Probability Analysis 
+; CHECK-NEXT:   Lazy Block Frequency Analysis 
+; CHECK-NEXT:   Optimization Remark Emitter
 ; CHECK-NEXT:   Expand Atomic instructions
 ; CHECK-NEXT:   Lower AMX intrinsics
 ; CHECK-NEXT:   Lower AMX type for load/store
Index: llvm/test/CodeGen/AMDGPU/llc-pipeline.ll
===
--- llvm/test/CodeGen/AMDGPU/llc-pipeline.ll
+++ llvm/test/CodeGen/AMDGPU/llc-pipeline.ll
@@ -44,6 +44,11 @@
 ; GCN-O0-NEXT:Lower OpenCL enqueued blocks
 ; GCN-O0-NEXT:Lower uses of LDS variables from non-kernel functions
 ; GCN-O0-NEXT:FunctionPass Manager
+; GCN-O0-NEXT:  Dominator Tree Construction
+; GCN-O0-NEXT:  Natural Loop Information
+; GCN-O0-NEXT:  Lazy Branch Probability Analysis
+; GCN-O0-NEXT:  Lazy Block Frequency Analysis
+; GCN-O0-NEXT:  Optimization Remark Emitter
 ; GCN-O0-NEXT:  Expand Atomic instructions
 ; GCN-O0-NEXT:  Lower constant intrinsics
 ; GCN-O0-NEXT:  Remove unreachable blocks from the CFG
@@ -180,6 +185,11 @@
 ; GCN-O1-NEXT:Lower uses of LDS variables from non-kernel functions
 ; GCN-O1-NEXT:FunctionPass Manager
 ; GCN-O1-NEXT:  Infer address spaces
+; GCN-O1-NEXT:  Dominator Tree Construction
+; GCN-O1-NEXT:  Natural Loop Information
+; GCN-O1-NEXT:  Lazy Branch Probability Analysis
+; GCN-O1-NEXT:  Lazy Block Frequency Analysis
+; GCN-O1-NEXT:  Optimization Remark Emitter
 ; GCN-O1-NEXT:  Expand Atomic instructions
 ; GCN-O1-NEXT:  AMDGPU Promote Alloca
 ; GCN-O1-NEXT:  Dominator Tree Construction
@@ -431,6 +441,11 @@
 ; GCN-O1-OPTS-NEXT:Lower uses of LDS variables from non-kernel functions
 ; GCN-O1-OPTS-NEXT:FunctionPass Manager
 ; GCN-O1-OPTS-NEXT:  Infer address spaces
+; GCN-O1-OPTS-NEXT:  Dominator Tree Construction
+; GCN-O1-OPTS-NEXT:  Natural Loop Information
+; GCN-O1-OPTS-NEXT:  Lazy Branch Probability Analysis
+; GCN-O1-OPTS-NEXT:  Lazy Block Frequency Analysis
+; GCN-O1-OPTS-NEXT:  Optimization Remark Emitter
 ; GCN-O1-OPTS-NEXT:  Expand Atomic instructions
 ; GCN-O1-OPTS-NEXT:  AMDGPU Promote Alloca
 ; GCN-O1-OPTS-NEXT:  Dominator Tree Construction
@@ -715,6 +730,11 @@
 ; GCN-O2-NEXT:Lower uses of LDS variables from non-kernel functions
 ; GCN-O2-NEXT:FunctionPass Manager
 ; GCN-O2-NEXT:  Infer

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Anshil Gandhi via Phabricator via cfe-commits]

gandhi21299 added a comment.

@rampitec should the unsafe check go in some pass later in the pipeline then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec added a comment.

In D106891#2921048 , @gandhi21299 
wrote:

> @rampitec should the unsafe check go in some pass later in the pipeline then?

No. The only place which has all the knowledge is 
`SITargetLowering::shouldExpandAtomicRMWInIR()`. That is where diagnostics 
shall be emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106909: [clang] Add clang builtins support for gfx90a

[Anshil Gandhi via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16212
+  case AMDGPU::BI__builtin_amdgcn_flat_atomic_fmax_f64: {
+Intrinsic::ID IID;
+llvm::Type *ArgTy = llvm::Type::getDoubleTy(getLLVMContext());

rampitec wrote:
> You do not need any of that code. You can directly map a builtin to intrinsic 
> in the IntrinsicsAMDGPU.td.
Sorry, I looked around for several days but I could not figure this out. Is 
there a concrete example?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106909

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

[PATCH] D106909: [clang] Add clang builtins support for gfx90a

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16212
+  case AMDGPU::BI__builtin_amdgcn_flat_atomic_fmax_f64: {
+Intrinsic::ID IID;
+llvm::Type *ArgTy = llvm::Type::getDoubleTy(getLLVMContext());

gandhi21299 wrote:
> rampitec wrote:
> > You do not need any of that code. You can directly map a builtin to 
> > intrinsic in the IntrinsicsAMDGPU.td.
> Sorry, I looked around for several days but I could not figure this out. Is 
> there a concrete example?
Every instantiation of `GCCBuiltin` in the `IntrinsicsAMDGPU.td`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106909

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

[PATCH] D106909: [clang] Add clang builtins support for gfx90a

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16212
+  case AMDGPU::BI__builtin_amdgcn_flat_atomic_fmax_f64: {
+Intrinsic::ID IID;
+llvm::Type *ArgTy = llvm::Type::getDoubleTy(getLLVMContext());

rampitec wrote:
> gandhi21299 wrote:
> > rampitec wrote:
> > > You do not need any of that code. You can directly map a builtin to 
> > > intrinsic in the IntrinsicsAMDGPU.td.
> > Sorry, I looked around for several days but I could not figure this out. Is 
> > there a concrete example?
> Every instantiation of `GCCBuiltin` in the `IntrinsicsAMDGPU.td`.
This is not true if the intrinsic requires type mangling. GCCBuiltin is too 
simple to handle it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106909

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

[PATCH] D106909: [clang] Add clang builtins support for gfx90a

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16212
+  case AMDGPU::BI__builtin_amdgcn_flat_atomic_fmax_f64: {
+Intrinsic::ID IID;
+llvm::Type *ArgTy = llvm::Type::getDoubleTy(getLLVMContext());

arsenm wrote:
> rampitec wrote:
> > gandhi21299 wrote:
> > > rampitec wrote:
> > > > You do not need any of that code. You can directly map a builtin to 
> > > > intrinsic in the IntrinsicsAMDGPU.td.
> > > Sorry, I looked around for several days but I could not figure this out. 
> > > Is there a concrete example?
> > Every instantiation of `GCCBuiltin` in the `IntrinsicsAMDGPU.td`.
> This is not true if the intrinsic requires type mangling. GCCBuiltin is too 
> simple to handle it
Yes, but these do not need it. All of these builtins are specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106909

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

[PATCH] D106909: [clang] Add clang builtins support for gfx90a

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16212
+  case AMDGPU::BI__builtin_amdgcn_flat_atomic_fmax_f64: {
+Intrinsic::ID IID;
+llvm::Type *ArgTy = llvm::Type::getDoubleTy(getLLVMContext());

rampitec wrote:
> arsenm wrote:
> > rampitec wrote:
> > > gandhi21299 wrote:
> > > > rampitec wrote:
> > > > > You do not need any of that code. You can directly map a builtin to 
> > > > > intrinsic in the IntrinsicsAMDGPU.td.
> > > > Sorry, I looked around for several days but I could not figure this 
> > > > out. Is there a concrete example?
> > > Every instantiation of `GCCBuiltin` in the `IntrinsicsAMDGPU.td`.
> > This is not true if the intrinsic requires type mangling. GCCBuiltin is too 
> > simple to handle it
> Yes, but these do not need it. All of these builtins are specific.
These intrinsics are all mangled based on the FP type


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106909

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

[PATCH] D106909: [clang] Add clang builtins support for gfx90a

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16212
+  case AMDGPU::BI__builtin_amdgcn_flat_atomic_fmax_f64: {
+Intrinsic::ID IID;
+llvm::Type *ArgTy = llvm::Type::getDoubleTy(getLLVMContext());

arsenm wrote:
> rampitec wrote:
> > arsenm wrote:
> > > rampitec wrote:
> > > > gandhi21299 wrote:
> > > > > rampitec wrote:
> > > > > > You do not need any of that code. You can directly map a builtin to 
> > > > > > intrinsic in the IntrinsicsAMDGPU.td.
> > > > > Sorry, I looked around for several days but I could not figure this 
> > > > > out. Is there a concrete example?
> > > > Every instantiation of `GCCBuiltin` in the `IntrinsicsAMDGPU.td`.
> > > This is not true if the intrinsic requires type mangling. GCCBuiltin is 
> > > too simple to handle it
> > Yes, but these do not need it. All of these builtins are specific.
> These intrinsics are all mangled based on the FP type
Ah, right. Intrinsics are mangled, builtins not. True. OK, this shall be code 
then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106909

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

[PATCH] D106909: [clang] Add clang builtins support for gfx90a

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16270
+llvm::Function *F = CGM.getIntrinsic(IID, {ArgTy});
+return Builder.CreateCall(F, {Addr, Val, ZeroI32, ZeroI32, ZeroI1});
+  }

Should we map flags since we already have them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106909

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Anshil Gandhi via Phabricator via cfe-commits]

gandhi21299 added a comment.

@rampitec Since remarks cannot be emitted in SIISelLowering because it isn't a 
pass, in what form can I emit the diagnostics in SIISelLowering?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec added a comment.

In D106891#2921096 , @gandhi21299 
wrote:

> @rampitec Since remarks cannot be emitted in SIISelLowering because it isn't 
> a pass, in what form can I emit the diagnostics in SIISelLowering?

You could pass ORE to the TLI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:597
+  OptimizationRemark Remark(DEBUG_TYPE, "Passed", RMW->getFunction());
+  Remark << "A hardware instruction was generated";
+  return Remark;

This is too strong of a statement here, although I think emitting anything here 
is useless


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Anshil Gandhi via Phabricator via cfe-commits]

gandhi21299 added a comment.

How can I construct an ORE to start off with? I don't think its appropriate to 
construct it in `shouldExpandAtomicRMWInsts(RMW)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Anshil Gandhi via Phabricator via cfe-commits]

gandhi21299 added inline comments.



Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:597
+  OptimizationRemark Remark(DEBUG_TYPE, "Passed", RMW->getFunction());
+  Remark << "A hardware instruction was generated";
+  return Remark;

arsenm wrote:
> This is too strong of a statement here, although I think emitting anything 
> here is useless
The only other option is SIISelLowering then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec added a comment.

In D106891#2921108 , @gandhi21299 
wrote:

> How can I construct an ORE to start off with? I don't think its appropriate 
> to construct it in `shouldExpandAtomicRMWInsts(RMW)`

You have already constructed it. You can just pass it to 
`shouldExpandAtomicRMWInsts`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

If you insist on making this a target remark (which I don't think it should be, 
and don't think it should be too specific about why the expansion happened), 
you have to pass through the ORE to the callback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Anshil Gandhi via Phabricator via cfe-commits]

gandhi21299 added a comment.

@rampitec That would mean changing the interfaces in TargetLowering(), 
AMDGPUISelLowering, and so on. Is that okay?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks when an FP atomic instruction is converted into a CAS loop or unsafe hardware instruction for GFX90A

[Anshil Gandhi via Phabricator via cfe-commits]

gandhi21299 added a comment.

Callback sounds way nicer, I will do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891

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

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Hi, sorry I'm late to the game. IWYU maintainer here.

I saw this patch mentioned in the LLVM Weekly newsletter and immediately 
thought: "wow, great, I have to build support in IWYU for that!".

I too prefer pragmas to magic comments, and I don't think `include_instead` 
necessarily needs to cover all IWYU pragmas 
(https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md).

Took me a while to get my head around it, but I see now that this is only 
supported for system headers. I think that makes sense for the compiler, 
otherwise it will be hard to guess which headers are allowed to include what. 
IWYU usually doesn't have that problem, as we analyze source files 
individually, and usually not headers independently.

My only concern was spelling -- in IWYU we need some handholding to know 
whether to use angled or quoted includes, but I see the quoting is part of the 
pragma, so that should be nice and useful.

Does this support the macro-able `_Pragma` syntax, so that users can be 
portable using something like:

  #ifdef __clang__
  #define INCLUDE_INSTEAD(headername) _Pragma("clang include_instead " ## 
headername)
  #else
  #define INCLUDE_INSTEAD(headername)
  #endif

? That might also make a nice testcase.

I'd love it if you could keep me in the loop if you want to extend this to 
something more general (using something like e.g. `IWYU pragma: friend`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106394

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

[PATCH] D107325: [clang-tidy] Fix command line is too long issue which breaks test on Windows

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Thanks!

Since the config file contents are fixed as far as I can tell, maybe we could 
instead just add a file with the right contents to 
`clang-tools-extra/test/clang-tidy/checkers/Inputs` and refer to that? (Use 
`%S/Inputs/myfile.txt`) (Alternatively you could use the `split-file` utility, 
but putting the file in Inputs is probably easier if you're new, and it's not 
any worse :) )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107325

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

[PATCH] D107125: [Diagnostic] Split 'qualifier on reference type has no effect' out into a new flag

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D107125#2920989 , @lunasorcery 
wrote:

> How does this look? I've got a 'control' test to demonstrate the diagnostic 
> as a child of ignored-qualifiers, as well as a test to demonstrate this 
> diagnostic can be disabled in isolation.

The test contents are great, but I had a suggestion for combining the test 
files.




Comment at: clang/test/SemaCXX/ignored-reference-qualifiers-disabled.cpp:1-20
+// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers 
-Wno-ignored-reference-qualifiers -verify
+
+const int scalar_c(); // expected-warning{{'const' type qualifier on return 
type has no effect}}
+volatile int scalar_v(); // expected-warning{{'volatile' type qualifier on 
return type has no effect}}
+const volatile int scalar_cv(); // expected-warning{{'const volatile' type 
qualifiers on return type have no effect}}
+
+// no warnings expected below here, as we've disabled 
ignored-reference-qualifiers.

Given that the test file contents are the same except for expected diagnostics, 
I would recommend combining them into a single test file with two `// RUN` 
lines. Something like this (untested, but hopefully it gives you the right 
idea):
```
// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers 
-Wno-ignored-reference-qualifiers -verify=both
// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers -verify=both,qual

const int scalar_c(); // both-warning{{'const' type qualifier on return type 
has no effect}}
volatile int scalar_v(); // both-warning{{'volatile' type qualifier on return 
type has no effect}}
const volatile int scalar_cv(); // both-warning{{'const volatile' type 
qualifiers on return type have no effect}}

typedef int& IntRef;

const IntRef ref_c(); // qual-warning{{'const' qualifier on reference type 
'IntRef' (aka 'int &') has no effect}}
volatile IntRef ref_v(); // qual-warning{{'volatile' qualifier on reference 
type 'IntRef' (aka 'int &') has no effect}}
const volatile IntRef ref_cv(); // qual-warning{{'const' qualifier on reference 
type 'IntRef' (aka 'int &') has no effect}} \
qual-warning{{'volatile' qualifier on reference 
type 'IntRef' (aka 'int &') has no effect}}

template
class container {
using value_type = T;
using reference  = value_type&;
reference get();
const reference get() const; // qual-warning{{'const' qualifier on 
reference type 'container::reference' (aka 'T &') has no effect}}
};
```


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

https://reviews.llvm.org/D107125

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

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: NoQ, rsmith, martong, vsavchenko.
ASDenysPetrov added a project: clang.
Herald added subscribers: manas, steakhal, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
ASDenysPetrov requested review of this revision.
Herald added a subscriber: cfe-commits.

Assuming that values of constant arrays never change, we can retrieve values 
for specific position(index) right from the initializer, if presented. Add 
support of handling `StringLiteral` in 
`RegionStoreManager::getConstantValFromConstArrayInitializer`. Retrieve a 
character code by index from `StringLiteral` which is an initializer of 
constant arrays in global and local scopes.
This patch also disables direct binding a string literal as `LazyCompoundVal` 
for constant arrays of local storage duration.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107339

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -437,10 +437,14 @@
 
   RegionBindingsRef removeSubRegionBindings(RegionBindingsConstRef B,
 const SubRegion *R);
-  Optional
-  getConstantValFromConstArrayInitializer(RegionBindingsConstRef B,
-  QualType ElemT, const VarRegion *VR,
-  const llvm::APSInt &Idx);
+  Optional getConstantValFromConstArrayInitializer(
+  RegionBindingsConstRef B, const VarRegion *VR, const llvm::APSInt &Idx,
+  QualType ElemT);
+  SVal getSValFromStringLiteralByIndex(const StringLiteral *SL,
+   const llvm::APSInt &Idx, QualType ElemT);
+  Optional getSValFromInitListExprByIndex(const InitListExpr *ILE,
+const llvm::APSInt &Idx,
+QualType ElemT);
 
 public: // Part of public interface to class.
 
@@ -1628,38 +1632,69 @@
 
   return Result;
 }
+
 Optional RegionStoreManager::getConstantValFromConstArrayInitializer(
-RegionBindingsConstRef B, QualType ElemT, const VarRegion *VR,
-const llvm::APSInt &Idx) {
+RegionBindingsConstRef B, const VarRegion *VR, const llvm::APSInt &Idx,
+QualType ElemT) {
   // Array should constant a const value or a value of a global initializer in
   // main().
   const VarDecl *VD = VR->getDecl();
   if (!VD->getType().isConstQualified() && !ElemT.isConstQualified() &&
   (!B.isMainAnalysis() || !VD->hasGlobalStorage()))
 return None;
+
   // Array should have an initialization list.
   const Expr *Init = VD->getAnyInitializer();
-  // FIXME: Support StringLiteral and CompoundLiteralExpr as well.
-  const auto *InitList = dyn_cast_or_null(Init);
-  if (!InitList)
+  if (!Init)
 return None;
-  // Choose a correct function depending on index signedness.
+
+  // Handle StringLiteral.
+  if (const auto *SL = dyn_cast(Init))
+return getSValFromStringLiteralByIndex(SL, Idx, ElemT);
+
+  // Handle InitListExpr.
+  if (const auto *ILE = dyn_cast(Init))
+return getSValFromInitListExprByIndex(ILE, Idx, ElemT);
+
+  // FIXME: Handle CompoundLiteralExpr.
+
+  return None;
+}
+
+SVal RegionStoreManager::getSValFromStringLiteralByIndex(
+const StringLiteral *SL, const llvm::APSInt &Idx, QualType ElemT) {
+  assert(SL && "StringLiteral should not be null");
+  // If index is out of bounds, return Undef.
+  const int64_t I = Idx.getExtValue();
+  if (Idx.isSigned() && I < 0)
+return UndefinedVal();
+  // Technically, only i == length is guaranteed to be null.
+  // However, such overflows should be caught before reaching this point;
+  // the only time such an access would be made is if a string literal was
+  // used to initialize a larger array.
+  uint32_t Code =
+  (static_cast(I) >= SL->getLength()) ? 0 : SL->getCodeUnit(I);
+  return svalBuilder.makeIntVal(Code, ElemT);
+}
+
+Optional RegionStoreManager::getSValFromInitListExprByIndex(
+const InitListExpr *ILE, const llvm::APSInt &Idx, QualType ElemT) {
+  assert(ILE && "InitListExpr should not be null");
   const int64_t I = Idx.getExtValue();
+  // Choose a correct function depending on index signedness.
   const Expr *E =
   Idx.isSigned()
-  ? InitList->getExprForConstArrayByRawIndex(I)
-  : InitList->getExprForConstArrayByRawIndex(static_cast(I));
+  ? ILE->getExprForConstArrayByRawIndex(I)
+  : ILE->getExprForConstArrayByRawIndex(static_cast(I));
   // If E is null, then the index is out of bounds. Return Undef.
   if (!E)
 return UndefinedVal();
-  // If E is the same as InitList, then there is no value specified
+  // If E is the same as ILE, then there is no value specified
 

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

TODO: add regression tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107339

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

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

That looks interesting!
Can you please add tests, though?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107339

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

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D107292#2920637 , @dblaikie wrote:

> Not a huge objection - but minor quandry: What's the motivation for this 
> patch? I don't know of any codebase that encourages/uses the alternative 
> tokens and I wonder if adding more usability to them is a worthwhile 
> investment in clang's codebase complexity, etc.

To me, the motivation comes from a signature like: `void foo(int and);` because 
it looks reasonable until you realize in C++ it means `void foo(int &&);` 
rather than declaring a parameter name of type `int`. What's worse, it can come 
up naturally in header files because it's a valid declaration in C if iso646.h 
is not included. So it's plausible to write the signature in a library exposing 
a C interface that changes the function signature when compiled in C++ -- this 
warning helps alert the programmer to such a situation.

I'm not aware of any popular coding guidelines that suggest using alternative 
tokens, but their use does show up in the wild.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107292

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

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D107339#2921896 , @vsavchenko 
wrote:

> That looks interesting!
> Can you please add tests, though?

Yes, I'll add them soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107339

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

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D107292#2920774 , @dblaikie wrote:

> In D107292#2920746 , @cjdb wrote:
>
>> In D107292#2920637 , @dblaikie 
>> wrote:
>>
>>> Not a huge objection - but minor quandry: What's the motivation for this 
>>> patch? I don't know of any codebase that encourages/uses the alternative 
>>> tokens and I wonder if adding more usability to them is a worthwhile 
>>> investment in clang's codebase complexity, etc.
>>
>> There are codebases that use them (all of my non-Google, non-LLVM code does, 
>> for example, and I'm not the sole user: just a loud one who's also in a 
>> position to patch tooling).
>
> Ah, any pointers to large open source projects that use this?

https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=bitand&search=Search

(Searching for 'and' is a bit less useful because of how much it shows up in 
assembly, comments, etc.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107292

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

[PATCH] D107269: [clan-format] detect function definitions more conservatively

[Owen Pan via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107269

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

[clang] 977bdf6 - Make simple requirements starting with requires ill-formed in in requirement body

[Aaron Ballman via cfe-commits]

Author: Corentin Jabot
Date: 2021-08-03T07:42:29-04:00
New Revision: 977bdf6f44edabb857bdff9ca249aa6eccb98e96

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

LOG: Make simple requirements starting with requires ill-formed in in 
requirement body

This patch implements P2092

Simple requirements in requirement body shall not start with requires.
A warning was already in place so we just turn this warning into an error.

In addition, we add tests to make sure typename is optional in
requirement-parameter-list as per the same paper.

Added: 


Modified: 
clang/include/clang/Basic/DiagnosticParseKinds.td
clang/lib/Parse/ParseExprCXX.cpp
clang/test/Parser/cxx2a-concepts-requires-expr.cpp
clang/www/cxx_status.html

Removed: 




diff  --git a/clang/include/clang/Basic/DiagnosticParseKinds.td 
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 7e4b0841e06b1..a4aa7fec4d557 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -806,10 +806,10 @@ def err_requires_expr_expected_type_constraint : Error<
 def err_requires_expr_simple_requirement_noexcept : Error<
   "'noexcept' can only be used in a compound requirement (with '{' '}' around "
   "the expression)">;
-def warn_requires_expr_in_simple_requirement : Warning<
-  "this requires expression will only be checked for syntactic validity; did "
+def err_requires_expr_in_simple_requirement : Error<
+  "requires expression in requirement body; did "
   "you intend to place it in a nested requirement? (add another 'requires' "
-  "before the expression)">, InGroup>;
+  "before the expression)">;
 
 def err_missing_dependent_template_keyword : Error<
   "use 'template' keyword to treat '%0' as a dependent template name">;

diff  --git a/clang/lib/Parse/ParseExprCXX.cpp 
b/clang/lib/Parse/ParseExprCXX.cpp
index f3d10b4a08895..3f357233f9bb4 100644
--- a/clang/lib/Parse/ParseExprCXX.cpp
+++ b/clang/lib/Parse/ParseExprCXX.cpp
@@ -3602,7 +3602,7 @@ ExprResult Parser::ParseRequiresExpression() {
   break;
 }
 if (!Expression.isInvalid() && PossibleRequiresExprInSimpleRequirement)
-  Diag(StartLoc, diag::warn_requires_expr_in_simple_requirement)
+  Diag(StartLoc, diag::err_requires_expr_in_simple_requirement)
   << FixItHint::CreateInsertion(StartLoc, "requires");
 if (auto *Req = Actions.ActOnSimpleRequirement(Expression.get()))
   Requirements.push_back(Req);

diff  --git a/clang/test/Parser/cxx2a-concepts-requires-expr.cpp 
b/clang/test/Parser/cxx2a-concepts-requires-expr.cpp
index d1a31b4f93ef1..d22f21b786f44 100644
--- a/clang/test/Parser/cxx2a-concepts-requires-expr.cpp
+++ b/clang/test/Parser/cxx2a-concepts-requires-expr.cpp
@@ -134,13 +134,13 @@ void r37(bool b) requires requires { 1 } {}
 // expected-error@-1 {{expected ';' at end of requirement}}
 
 bool r38 = requires { requires { 1; }; };
-// expected-warning@-1 {{this requires expression will only be checked for 
syntactic validity; did you intend to place it in a nested requirement? (add 
another 'requires' before the expression)}}
+// expected-error@-1 {{requires expression in requirement body; did you intend 
to place it in a nested requirement? (add another 'requires' before the 
expression)}}
 
 bool r39 = requires { requires () { 1; }; };
-// expected-warning@-1 {{this requires expression will only be checked for 
syntactic validity; did you intend to place it in a nested requirement? (add 
another 'requires' before the expression)}}
+// expected-error@-1 {{requires expression in requirement body; did you intend 
to place it in a nested requirement? (add another 'requires' before the 
expression)}}
 
 bool r40 = requires { requires (int i) { i; }; };
-// expected-warning@-1 {{this requires expression will only be checked for 
syntactic validity; did you intend to place it in a nested requirement? (add 
another 'requires' before the expression)}}
+// expected-error@-1 {{requires expression in requirement body; did you intend 
to place it in a nested requirement? (add another 'requires' before the 
expression)}}
 
 bool r41 = requires { requires (); };
 // expected-error@-1 {{expected expression}}

diff  --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html
index 2ad2047f68d00..0e78d63220e49 100755
--- a/clang/www/cxx_status.html
+++ b/clang/www/cxx_status.html
@@ -934,7 +934,7 @@ C++20 implementation status
   

 https://wg21.link/p1972r0";>P1972R0
-No
+No
   
   
 https://wg21.link/p1980r0";>P1980R0
@@ -944,9 +944,11 @@ C++20 implementation status
   
   
 https://wg21.link/p2092r0";>P2092R0
+Clang 13
   
   
 https://wg21.li

[PATCH] D106252: Make simple requirements starting with requires ill-formed in in requirement body

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

I've commit on your behalf in 977bdf6f44edabb857bdff9ca249aa6eccb98e96 
, thank 
you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106252

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

[PATCH] D107325: [clang-tidy] Fix command line is too long issue which breaks test on Windows

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 363698.
dougpuob added a comment.
Herald added a subscriber: aheejin.

- Moved config content of regression test to 
`clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation*/.clang-tidy`,
 then refer to those files in tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107325

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation1/.clang-tidy
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation2/.clang-tidy
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation-cfgfile.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -1,62 +1,5 @@
-// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
-// RUN:   -config='{ CheckOptions: [ \
-// RUN: { key: readability-identifier-naming.AbstractClassCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ClassCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ClassConstantCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ClassMemberCase , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstantCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstantMemberCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstantParameterCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstantPointerParameterCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstexprVariableCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.EnumConstantCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.GlobalConstantCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.GlobalConstantPointerCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.GlobalPointerCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.GlobalVariableCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.LocalConstantCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.LocalConstantPointerCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.LocalPointerCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.LocalVariableCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.MemberCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ParameterCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.PointerParameterCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.PrivateMemberCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ProtectedMemberCase , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.PublicMemberCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ScopedEnumConstantCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.StaticConstantCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.StaticVariableCase  , value: CamelCase }, \
-// RUN: { key: readabilit

[PATCH] D107325: [clang-tidy] Fix command line is too long issue which breaks test on Windows

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D107325#2921844 , @thakis wrote:

> Thanks!
>
> Since the config file contents are fixed as far as I can tell, maybe we could 
> instead just add a file with the right contents to 
> `clang-tools-extra/test/clang-tidy/checkers/Inputs` and refer to that? (Use 
> `%S/Inputs/myfile.txt`) (Alternatively you could use the `split-file` 
> utility, but putting the file in Inputs is probably easier if you're new, and 
> it's not any worse :) )

Hi @thakis:
It's a really good idea, thank you for the suggestion. I just upload the latest 
diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107325

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

[PATCH] D107325: [clang-tidy] Fix command line is too long issue which breaks test on Windows

[Nico Weber via Phabricator via cfe-commits]

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

Thanks!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation1/.clang-tidy:3
+CheckOptions:
+  - { key: readability-identifier-naming.AbstractClassCase 
  , value: CamelCase }
+  - { key: readability-identifier-naming.ClassCase 
  , value: CamelCase }

can we drop all the spaces here? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107325

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

[PATCH] D107325: [clang-tidy] Fix command line is too long issue which breaks test on Windows

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In general, would it be possible to specify the configuration file as a custom 
path, and not use the hidden filename `.clang-tidy` in an otherwise empty 
directory? I think there's a `--config` flag on Tidy's interface for giving any 
file from any path for this. So it could be `config1.yaml` or something like 
that instead.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation1/.clang-tidy:1
+Checks: readability-identifier-naming
+CheckOptions:

I do not think this file should have the //execute// bit set...



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation1/.clang-tidy:3
+CheckOptions:
+  - { key: readability-identifier-naming.AbstractClassCase 
  , value: CamelCase }
+  - { key: readability-identifier-naming.ClassCase 
  , value: CamelCase }

thakis wrote:
> can we drop all the spaces here? :)
And maybe even the object brackets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107325

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

[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

[Jim Lin via Phabricator via cfe-commits]

Jim added a comment.

Kindly ping? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588

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

[PATCH] D106343: [OpenCL] Support cl_ext_float_atomics

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Just to make sure you are aware Clang doesn't use this header by default, so 
the upstream users won't be able to call those functions unless you add them 
into `OpenCLBuiltins.td`:
https://clang.llvm.org/docs/OpenCLSupport.html#opencl-builtins

This header is only accessible via the frontend options: 
https://clang.llvm.org/docs/OpenCLSupport.html#cmdoption-finclude-default-header




Comment at: clang/lib/Headers/opencl-c.h:13435
+   memory_scope scope);
+#endif
+#if defined(__opencl_c_ext_fp32_local_atomic_min_max)

Can you annotate the `#endif`s with a comment describing what they correspond 
to. i.e. something like:


```
#endif //defined(__opencl_c_ext_fp32_global_atomic_min_max)
```


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

https://reviews.llvm.org/D106343

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

[PATCH] D106614: [Clang] add btf_tag attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6841
+  if (auto *Rec = dyn_cast(D)) {
+if (!Rec->isCompleteDefinition()) {
+  S.Diag(AL.getLoc(), diag::warn_btftag_attribute_fwd_decl_ignored) << Str;

yonghong-song wrote:
> aaron.ballman wrote:
> > Does this work for code like:
> > ```
> > struct __attribute__((btf_tag(""))) S {
> >   int x;
> > };
> > ```
> > (I didn't see a test case for the attribute being written in that location.)
> Yes, I missed this one as typically I (and kernel developers) write the 
> attribute at the end of definition. I just checked that if we put attribute 
> before the struct name, when the attribute is handled/merged, it is actually 
> an incomplete definition.
I kind of thought that might be the case. I think you'll want to also test 
`!Rec->isBeingDefined()`



Comment at: clang/test/Sema/attr-btf_tag.c:31
+
+int __tag2 foo(struct t1 *arg); // expected-warning {{attribute 
'btf_tag("tag2")' ignored as not in later redeclaration or definition}}
+int __tag foo(struct t1 *arg);

yonghong-song wrote:
> aaron.ballman wrote:
> > This looks backwards to me -- I think the initial declaration of `foo()` is 
> > fine and shouldn't be diagnosed, it's the subsequent declarations of 
> > `foo()` with a different `btf_tag` argument that should be diagnosed.
> > 
> > I think additional test cases for these semantics is:
> > ```
> > void bar();
> > void __tag bar(); // (I think I understood that you want this to diagnose.)
> > 
> > void __tag bar();
> > void bar(); // (But that you don't want this to diagnose.)
> > ```
> > 
> There are a little complication here. Maybe you can give some advice.
> We could have code like
>D1: void __tag1 __tag2 bar();
>D2: void bar();
> The reason is that we explicitly allow multiple btf_tag's in the same 
> declaration and this is exactly our use case.
> 
> Current merge*Attribute() framework will take one attribute at a time.
> For example, first we will do
> mergeBTFTagAttribute(D2, __tag1):
>   Here, we add __tag1 to D2, so we have
>D2: void __tag1 bar();
> we then do
> mergeBTFTagAttribute(D2, __tag2):
>   but D2 already has a __tag1 which is incompatible with __tag2, and
>   we will issue an error here. But it is incorrect to issue an error
>   since the correct semantics is to inherit both __tag1 and __tag2 for D2.
> 
> Let me take a look at the code how to best handle such cases. Please also let 
> me know if you have any suggestions. thanks!
> 
> A little bit more. The following are possible cases to check:
>(1)  void bar();
>  void __tag1 __tag2 bar();  // fail
>(2).  void __tag1 bar();
>  void __tag1 __tag2 bar(); // fail
>(3)   void __tag1 __tag2 bar();
>  void __tag1 __tag2 bar();  // succeed
>(4). void __tag1 __tag2 __tag3 bar();
>  void __tag1 __tag2 bar();   // fail
>(5). void __tag3 bar();
>  void __tag1 __tag2 bar(); // fail
>(6). void __tag1 __tag2 bar();
>  void bar()  // succeed
> 
> Basically, for two same declaration except attributes, D1, D2,
>no error/warning only if D2 has no btf_tag attributes, or
>D1 and D2 have identical btf_tag attributes.
> Do this sound reasonable?
> Let me take a look at the code how to best handle such cases. Please also let 
> me know if you have any suggestions. thanks!

Oh, that's... interesting! You want to accept multiple, potentially separate 
attribute specifiers on the initial declaration, but not allow any new 
attributes on redeclarations or the definition. Btw, there are some tests 
missing from your above comment. You also need to think about:
```
void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void bar(); // succeed

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // succeed

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"))) bar(); // fail

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"), btf_tag("two"), btf_tag("three"))) bar(); 
// fail

[[clang::btf_tag("one")]] void bar [[clang::btf_tag("two")]] (); // ok
void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // succeed
```
I don't think we have any attributes that behave like that currently. Can you 
remind me: what is the concern if the attributes are additive on redeclaration?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614

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

[clang] 80c17bb - This feature is not in Clang 13 and only has partial support

[Aaron Ballman via cfe-commits]

Author: Aaron Ballman
Date: 2021-08-03T08:21:15-04:00
New Revision: 80c17bb29838bee0d67a7bc2c775a8a91d69ac2d

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

LOG: This feature is not in Clang 13 and only has partial support

Added: 


Modified: 
clang/www/cxx_status.html

Removed: 




diff  --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html
index 0e78d63220e49..92bc95fc51e9d 100755
--- a/clang/www/cxx_status.html
+++ b/clang/www/cxx_status.html
@@ -944,7 +944,7 @@ C++20 implementation status
   
   
 https://wg21.link/p2092r0";>P2092R0
-Clang 13
+Partial
   
   
 https://wg21.link/p2113r0";>P2113R0



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

[PATCH] D107151: [OpenCL] Reduce duplicate defs by using multiclasses; NFC

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Looks very neat!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107151

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

[PATCH] D107318: [OpenCL] allow generic address and non-generic defs for CL3.0

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107318

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

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 363713.
ASDenysPetrov added a comment.

Added tests.


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

https://reviews.llvm.org/D107339

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/initialization.cpp

Index: clang/test/Analysis/initialization.cpp
===
--- clang/test/Analysis/initialization.cpp
+++ clang/test/Analysis/initialization.cpp
@@ -295,3 +295,68 @@
   int x = 42;
   int res = local_arr[x]; // expected-warning{{garbage or undefined}}
 }
+
+const char glob_str_arr1[8] = "1234";
+void glob_str_arr_index1() {
+  clang_analyzer_eval(glob_str_arr1[0] == '1');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_str_arr1[1] == '2');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_str_arr1[2] == '3');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_str_arr1[3] == '4');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_str_arr1[4] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_str_arr1[5] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_str_arr1[6] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_str_arr1[7] == '\0'); // expected-warning{{TRUE}}
+}
+
+void glob_str_arr_out_of_bound_index1() {
+  int x = -42;
+  int res = glob_str_arr1[x]; // expected-warning{{garbage or undefined}}
+}
+
+void glob_str_arr_out_of_bound_index2() {
+  int x = 42;
+  // FIXME: This should warn about "garbage or undefined".
+  int res = glob_str_arr1[x]; // no-warning
+  // FIXME-cont: `res` is `\0` but should be undefined.
+  clang_analyzer_eval(res == '\0'); // expected-warning{{TRUE}}
+}
+
+void local_str_arr_index1() {
+  const char str_arr[8] = "1234";
+  clang_analyzer_eval(str_arr[0] == '1');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(str_arr[1] == '2');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(str_arr[2] == '3');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(str_arr[3] == '4');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(str_arr[4] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str_arr[5] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str_arr[6] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str_arr[7] == '\0'); // expected-warning{{TRUE}}
+}
+
+void local_str_arr_out_of_bound_index3() {
+  const char str_arr[8] = "1234";
+  int x = -42;
+  int res = str_arr[x]; // expected-warning{{garbage or undefined}}
+}
+
+void local_str_arr_out_of_bound_index4() {
+  const char str_arr[8] = "1234";
+  int x = 42;
+  // FIXME: This should warn about "garbage or undefined".
+  int res = str_arr[x]; // no-warning
+  // FIXME-cont: `res` is `\0` but should be undefined.
+  clang_analyzer_eval(res == '\0'); // expected-warning{{TRUE}}
+}
+
+void ptr_local_str_arr_index1() {
+  const char str_arr[8] = "1234";
+  const char *ptr = str_arr;
+  clang_analyzer_eval(ptr[0] == '1');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[1] == '2');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[2] == '3');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[3] == '4');  // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[4] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[5] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[6] == '\0'); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[7] == '\0'); // expected-warning{{TRUE}}
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -437,10 +437,14 @@
 
   RegionBindingsRef removeSubRegionBindings(RegionBindingsConstRef B,
 const SubRegion *R);
-  Optional
-  getConstantValFromConstArrayInitializer(RegionBindingsConstRef B,
-  QualType ElemT, const VarRegion *VR,
-  const llvm::APSInt &Idx);
+  Optional getConstantValFromConstArrayInitializer(
+  RegionBindingsConstRef B, const VarRegion *VR, const llvm::APSInt &Idx,
+  QualType ElemT);
+  SVal getSValFromStringLiteralByIndex(const StringLiteral *SL,
+   const llvm::APSInt &Idx, QualType ElemT);
+  Optional getSValFromInitListExprByIndex(const InitListExpr *ILE,
+const llvm::APSInt &Idx,
+QualType ElemT);
 
 public: // Part of public interface to class.
 
@@ -1628,38 +1632,69 @@
 
   return Result;
 }
+
 Optional RegionStoreManager::getConstantValFromConstArrayInitializer(
-RegionBindingsConstRef B, QualType ElemT, const VarRegion *VR,
-const llvm::APSInt &Idx) {
+RegionBindingsConstRef B, const VarRegion *VR, const llvm::APSInt &Idx,
+Qual

[PATCH] D106908: Improve UBSan documentation

[Diane Meirowitz via Phabricator via cfe-commits]

DianeMeirowitz added a comment.

Mitch,

OK and thanks for submitting it!

Diane

On 8/2/21, 5:55 PM, "Mitch Phillips via Phabricator" 
 wrote:

  hctim added a comment.
  
  In D106908#2910131 
, @DianeMeirowitz wrote:
  
  > I don't agree with the phrasing : "Array subscript out of bounds, when the 
bounds can be statically determined". It is long, and I think it may confuse 
people who don't read language standards and also as far as I know, neither C 
nor C++ has a true dynamic array type. Dynamic arrays are declared as pointers, 
not arrays. So I suggest just keeping my original simple phrasing "Array 
subscript out of bounds". But if you feel strongly about this, go ahead.
  
  Indirection can kill the bounds tracking, so we normally add the caveat that 
"the bounds can be statically determined". For example, this simple case 
escapes ubsan-bounds (but not asan):
  
int f(int y[]) {
  return y[1];
}
  
int main() {
  int x[1];
  return f(x);
}
  
  I'll submit with the nit.
  
  
  CHANGES SINCE LAST ACTION

https://urldefense.com/v3/__https://reviews.llvm.org/D106908/new/__;!!ACWV5N9M2RV99hQ!ezsHkymW9n_oArVw48GURjA0PCnyK2zxkbZU2B9adp3WEpn3KvcVau7Y0fo6b0vh3Zht$
 
  
  
https://urldefense.com/v3/__https://reviews.llvm.org/D106908__;!!ACWV5N9M2RV99hQ!ezsHkymW9n_oArVw48GURjA0PCnyK2zxkbZU2B9adp3WEpn3KvcVau7Y0fo6b5X0CPtC$


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106908

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

[PATCH] D107304: [clangd][query-driver] Extract GCC version from the driver output

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Yes, there are quite a number of small differences in builtin support and even 
certain macros that just invite even more trouble than this. This is IMO 
begging for hard to trace down errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107304

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

[PATCH] D107325: [clang-tidy] Fix command line is too long issue which breaks test on Windows

[Douglas Chen via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation1/.clang-tidy:1
+Checks: readability-identifier-naming
+CheckOptions:

whisperity wrote:
> I do not think this file should have the //execute// bit set...
I will change it to 664.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation1/.clang-tidy:3
+CheckOptions:
+  - { key: readability-identifier-naming.AbstractClassCase 
  , value: CamelCase }
+  - { key: readability-identifier-naming.ClassCase 
  , value: CamelCase }

whisperity wrote:
> thakis wrote:
> > can we drop all the spaces here? :)
> And maybe even the object brackets.
Sure.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation1/.clang-tidy:3
+CheckOptions:
+  - { key: readability-identifier-naming.AbstractClassCase 
  , value: CamelCase }
+  - { key: readability-identifier-naming.ClassCase 
  , value: CamelCase }

dougpuob wrote:
> whisperity wrote:
> > thakis wrote:
> > > can we drop all the spaces here? :)
> > And maybe even the object brackets.
> Sure.
OK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107325

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

[PATCH] D107325: [clang-tidy] Fix command line is too long issue which breaks test on Windows

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 363717.
dougpuob marked 3 inline comments as done.
dougpuob added a comment.

- Improved code review suggestions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107325

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation1/.clang-tidy
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation2/.clang-tidy
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation-cfgfile.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -1,62 +1,5 @@
-// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
-// RUN:   -config='{ CheckOptions: [ \
-// RUN: { key: readability-identifier-naming.AbstractClassCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ClassCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ClassConstantCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ClassMemberCase , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstantCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstantMemberCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstantParameterCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstantPointerParameterCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ConstexprVariableCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.EnumConstantCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.GlobalConstantCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.GlobalConstantPointerCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.GlobalPointerCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.GlobalVariableCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.LocalConstantCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.LocalConstantPointerCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.LocalPointerCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.LocalVariableCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.MemberCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ParameterCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.PointerParameterCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.PrivateMemberCase   , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ProtectedMemberCase , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.PublicMemberCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.ScopedEnumConstantCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.StaticConstantCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.StaticVariableCase  , value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.VariableCase, value: CamelCase }, \
-// RUN: { key: readability-identifier-naming.Abstr

[PATCH] D107347: [Sema] haveSameParameterTypes - fix repeated isNull() test

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon created this revision.
RKSimon added a reviewer: rsmith.
RKSimon requested review of this revision.
Herald added a project: clang.

As reported on https://pvs-studio.com/en/blog/posts/cpp/0771/ (Snippet 2) - 
(and mentioned on rGdc4259d5a38409 
) we are 
repeating the T1.isNull() check instead of checking T2.isNull() as well.

I'm not familiar with this code but it doesn't look like we have any test 
coverage that relies on the null tests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107347

Files:
  clang/lib/Sema/SemaOverload.cpp


Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -9525,7 +9525,7 @@
   for (unsigned I = 0; I != NumParams; ++I) {
 QualType T1 = NextParam(F1, I1, I == 0);
 QualType T2 = NextParam(F2, I2, I == 0);
-if (!T1.isNull() && !T1.isNull() && !Context.hasSameUnqualifiedType(T1, 
T2))
+if (!T1.isNull() && !T2.isNull() && !Context.hasSameUnqualifiedType(T1, 
T2))
   return false;
   }
   return true;


Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -9525,7 +9525,7 @@
   for (unsigned I = 0; I != NumParams; ++I) {
 QualType T1 = NextParam(F1, I1, I == 0);
 QualType T2 = NextParam(F2, I2, I == 0);
-if (!T1.isNull() && !T1.isNull() && !Context.hasSameUnqualifiedType(T1, T2))
+if (!T1.isNull() && !T2.isNull() && !Context.hasSameUnqualifiedType(T1, T2))
   return false;
   }
   return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107349: [Matrix] Overload stride arg in matrix.columnwise.load/store.

[Florian Hahn via Phabricator via cfe-commits]

fhahn created this revision.
fhahn added reviewers: rjmccall, anemet, thegameg, erichkeane.
Herald added subscribers: dexonsmith, jdoerfert, tschuett, hiraditya.
fhahn requested review of this revision.
Herald added projects: clang, LLVM.

This patch adjusts the intrinsics definition of
llvm.matrix.column.major.load and llvm.matrix.column.major.store to
allow overloading the type of the stride. The bitwidth of the stride is
used to perform the offset computation.

This fixes a crash when using __builtin_matrix_column_major_load or
__builtin_matrix_column_major_store on 32 bit platforms. The stride argument
of the builtins are defined as `size_t`, which is 32 bits wide on 32 bit
platforms.

Note that we still perform offset computations with 64 bit width on 32
bit platforms for accesses that do not take a user-specified stride.
This can be fixed separately.

Fixes PR51304.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107349

Files:
  clang/test/CodeGen/matrix-type-builtins.c
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/CodeGenObjC/matrix-type-builtins.m
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/include/llvm/IR/MatrixBuilder.h
  llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
  llvm/test/Transforms/LowerMatrixIntrinsics/strided-load-double.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/strided-store-double.ll
  llvm/test/Verifier/matrix-intrinsics.ll

Index: llvm/test/Verifier/matrix-intrinsics.ll
===
--- llvm/test/Verifier/matrix-intrinsics.ll
+++ llvm/test/Verifier/matrix-intrinsics.ll
@@ -39,11 +39,11 @@
 ; CHECK-NEXT: Result of a matrix operation does not fit in the returned vector!
 ; CHECK-NEXT: immarg operand has non-immediate parameter
 ; CHECK-NEXT: i32 %arg
-; CHECK-NEXT:   %result.3 = call <6 x float> @llvm.matrix.column.major.load.v6f32(float* %n, i64 2, i1 true, i32 3, i32 %arg)
-  %result.0 = call <4 x float> @llvm.matrix.column.major.load.v4f32(float* %m, i64 0, i1 false, i32 0, i32 0)
-  %result.1 = call <4 x float> @llvm.matrix.column.major.load.v4f32(float* %m, i64 2, i1 false, i32 1, i32 2)
-  %result.2 = call <6 x float> @llvm.matrix.column.major.load.v6f32(float* %n, i64 2, i1 true, i32 3, i32 3)
-  %result.3 = call <6 x float> @llvm.matrix.column.major.load.v6f32(float* %n, i64 2, i1 true, i32 3, i32 %arg)
+; CHECK-NEXT:   %result.3 = call <6 x float> @llvm.matrix.column.major.load.v6f32.i64(float* %n, i64 2, i1 true, i32 3, i32 %arg)
+  %result.0 = call <4 x float> @llvm.matrix.column.major.load.v4f32.i64(float* %m, i64 0, i1 false, i32 0, i32 0)
+  %result.1 = call <4 x float> @llvm.matrix.column.major.load.v4f32.i64(float* %m, i64 2, i1 false, i32 1, i32 2)
+  %result.2 = call <6 x float> @llvm.matrix.column.major.load.v6f32.i64(float* %n, i64 2, i1 true, i32 3, i32 3)
+  %result.3 = call <6 x float> @llvm.matrix.column.major.load.v6f32.i64(float* %n, i64 2, i1 true, i32 3, i32 %arg)
   ret <4 x float> %result.1
 }
 
@@ -52,10 +52,10 @@
 ; CHECK-NEXT: Result of a matrix operation does not fit in the returned vector!
 ; CHECK-NEXT: Result of a matrix operation does not fit in the returned vector!
 ; CHECK-NEXT: Result of a matrix operation does not fit in the returned vector!
-  call void @llvm.matrix.column.major.store.v4f32(<4 x float> zeroinitializer, float* %m, i64 0, i1 false, i32 0, i32 0)
-  call void @llvm.matrix.column.major.store.v4f32(<4 x float> zeroinitializer, float* %m, i64 2, i1 false, i32 1, i32 2)
-  call void @llvm.matrix.column.major.store.v6f32(<6 x float> zeroinitializer, float* %n, i64 2, i1 false, i32 3, i32 3)
-  call void @llvm.matrix.column.major.store.v6f32(<6 x float> zeroinitializer, float* %n, i64 %arg, i1 false, i32 3, i32 3)
+  call void @llvm.matrix.column.major.store.v4f32.i64(<4 x float> zeroinitializer, float* %m, i64 0, i1 false, i32 0, i32 0)
+  call void @llvm.matrix.column.major.store.v4f32.i64(<4 x float> zeroinitializer, float* %m, i64 2, i1 false, i32 1, i32 2)
+  call void @llvm.matrix.column.major.store.v6f32.i64(<6 x float> zeroinitializer, float* %n, i64 2, i1 false, i32 3, i32 3)
+  call void @llvm.matrix.column.major.store.v6f32.i64(<6 x float> zeroinitializer, float* %n, i64 %arg, i1 false, i32 3, i32 3)
   ret void
 }
 
@@ -94,18 +94,18 @@
 ; CHECK-NEXT: Intrinsic has incorrect argument type!
 ; CHECK-NEXT: <4 x float> (i32*, i64, i1, i32, i32)* @llvm.matrix.column.major.load.v4f32.pi32
 ; CHECK-NEXT: Intrinsic has incorrect argument type!
-; CHECK-NEXT: <4 x i32> (float*, i64, i1, i32, i32)* @llvm.matrix.column.major.load.v4i32
+; CHECK-NEXT: <4 x i32> (float*, i64, i1, i32, i32)* @llvm.matrix.column.major.load.v4i32.i64
 ;
   %result.0 = call <4 x float> @llvm.matrix.column.major.load.v4f32.pi32(i32* %m, i64 2, i1 false, i32 2, i32 2)
-  %result.1 = call <4 x i32> @llvm.matrix.column.major.load.v4i32(float* %n, i64 2, i1 false, i32 2, i32 2)
+  %result.1 = call <4 x i32> @llvm.matrix.column.major.lo

[PATCH] D107311: [clang] fix canonicalization of nested name specifiers

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov created this revision.
mizvekov edited the summary of this revision.
mizvekov updated this revision to Diff 363710.
mizvekov edited the summary of this revision.
mizvekov added a comment.
mizvekov updated this revision to Diff 363719.
mizvekov updated this revision to Diff 363722.
mizvekov published this revision for review.
mizvekov added a reviewer: rsmith.
mizvekov added a subscriber: Quuxplusone.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

.


mizvekov added a comment.

.


mizvekov added a comment.

.


When canonicalizing nested name spcifiers of the type kind,
the prefix for 'DependentTemplateSpecialization' types was being
dropped, leading to malformed types which would cause failures
when rebuilding template names.

Signed-off-by: Matheus Izvekov 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107311

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp

Index: clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp
===
--- clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp
+++ clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp
@@ -81,4 +81,23 @@
 requires true struct S4; // expected-note {{template is declared here}}
 template 
 requires true struct S4; // expected-error {{class template partial specialization is not more specialized than the primary template}}
+
+struct X {
+  template struct Y {
+using type = int;
+  };
+};
+
+template concept C1 = sizeof(T) != 0;
+template concept C2 = C1::type>;
+
+template requires C1 void t1() = delete;  // expected-note {{candidate function}}
+template requires C1 && C2 void t1() = delete; // expected-note {{candidate function}}
+template void t1();
+void t1() { t1(); } // expected-error {{call to deleted function 't1'}}
+
+template requires C1 void t2() {}; // expected-note 2 {{candidate function}} 
+template requires C2 void t2() {}; // expected-note 2 {{candidate function}}
+template void t2(); // expected-error {{partial ordering for explicit instantiation of 't2' is ambiguous}}
+void t2() { t2(); } // expected-error {{call to 't2' is ambiguous}}
 } // namespace PR47174
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1079,7 +1079,7 @@
   return Param;
 
 // Check the template argument itself.
-if (CheckTemplateArgument(Param, DefaultTInfo)) {
+if (CheckTemplateArgument(DefaultTInfo)) {
   Param->setInvalidDecl();
   return Param;
 }
@@ -5042,7 +5042,7 @@
   }
   }
 
-  if (CheckTemplateArgument(Param, TSI))
+  if (CheckTemplateArgument(TSI))
 return true;
 
   // Add the converted template type argument.
@@ -5661,7 +5661,7 @@
   TemplateArgumentListInfo NewArgs = TemplateArgs;
 
   // Make sure we get the template parameter list from the most
-  // recentdeclaration, since that is the only one that has is guaranteed to
+  // recent declaration, since that is the only one that is guaranteed to
   // have all the default template argument information.
   TemplateParameterList *Params =
   cast(Template->getMostRecentDecl())
@@ -6208,8 +6208,7 @@
 ///
 /// This routine implements the semantics of C++ [temp.arg.type]. It
 /// returns true if an error occurred, and false otherwise.
-bool Sema::CheckTemplateArgument(TemplateTypeParmDecl *Param,
- TypeSourceInfo *ArgInfo) {
+bool Sema::CheckTemplateArgument(TypeSourceInfo *ArgInfo) {
   assert(ArgInfo && "invalid TypeSourceInfo");
   QualType Arg = ArgInfo->getType();
   SourceRange SR = ArgInfo->getTypeLoc().getSourceRange();
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -12472,6 +12472,8 @@
 return false;
   }
 
+  const NestedNameSpecifier *CNNS =
+  Context.getCanonicalNestedNameSpecifier(Qual);
   for (LookupResult::iterator I = Prev.begin(), E = Prev.end(); I != E; ++I) {
 NamedDecl *D = *I;
 
@@ -12497,8 +12499,7 @@
 // using decls differ if they name different scopes (but note that
 // template instantiation can cause this check to trigger when it
 // didn't before instantiation).
-if (Context.getCanonicalNestedNameSpecifier(Qual) !=
-Context.getCanonicalNestedNameSpecifier(DQual))
+if (CNNS != Context.getCanonicalNestedNameSpecifier(DQual))
   continue;
 
 Diag(NameLoc, diag::err_using_decl_redeclaration) << SS.getRange();
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6089,9 +6089,11 @@
  

[PATCH] D107311: [clang] fix canonicalization of nested name specifiers

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 363725.
mizvekov edited the summary of this revision.
mizvekov added a comment.

.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107311

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp

Index: clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp
===
--- clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp
+++ clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp
@@ -81,4 +81,23 @@
 requires true struct S4; // expected-note {{template is declared here}}
 template 
 requires true struct S4; // expected-error {{class template partial specialization is not more specialized than the primary template}}
+
+struct X {
+  template struct Y {
+using type = int;
+  };
+};
+
+template concept C1 = sizeof(T) != 0;
+template concept C2 = C1::type>;
+
+template requires C1 void t1() = delete;  // expected-note {{candidate function}}
+template requires C1 && C2 void t1() = delete; // expected-note {{candidate function}}
+template void t1();
+void t1() { t1(); } // expected-error {{call to deleted function 't1'}}
+
+template requires C1 void t2() {}; // expected-note 2 {{candidate function}} 
+template requires C2 void t2() {}; // expected-note 2 {{candidate function}}
+template void t2(); // expected-error {{partial ordering for explicit instantiation of 't2' is ambiguous}}
+void t2() { t2(); } // expected-error {{call to 't2' is ambiguous}}
 } // namespace PR47174
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1079,7 +1079,7 @@
   return Param;
 
 // Check the template argument itself.
-if (CheckTemplateArgument(Param, DefaultTInfo)) {
+if (CheckTemplateArgument(DefaultTInfo)) {
   Param->setInvalidDecl();
   return Param;
 }
@@ -5042,7 +5042,7 @@
   }
   }
 
-  if (CheckTemplateArgument(Param, TSI))
+  if (CheckTemplateArgument(TSI))
 return true;
 
   // Add the converted template type argument.
@@ -5661,7 +5661,7 @@
   TemplateArgumentListInfo NewArgs = TemplateArgs;
 
   // Make sure we get the template parameter list from the most
-  // recentdeclaration, since that is the only one that has is guaranteed to
+  // recent declaration, since that is the only one that is guaranteed to
   // have all the default template argument information.
   TemplateParameterList *Params =
   cast(Template->getMostRecentDecl())
@@ -6208,8 +6208,7 @@
 ///
 /// This routine implements the semantics of C++ [temp.arg.type]. It
 /// returns true if an error occurred, and false otherwise.
-bool Sema::CheckTemplateArgument(TemplateTypeParmDecl *Param,
- TypeSourceInfo *ArgInfo) {
+bool Sema::CheckTemplateArgument(TypeSourceInfo *ArgInfo) {
   assert(ArgInfo && "invalid TypeSourceInfo");
   QualType Arg = ArgInfo->getType();
   SourceRange SR = ArgInfo->getTypeLoc().getSourceRange();
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -12472,6 +12472,8 @@
 return false;
   }
 
+  const NestedNameSpecifier *CNNS =
+  Context.getCanonicalNestedNameSpecifier(Qual);
   for (LookupResult::iterator I = Prev.begin(), E = Prev.end(); I != E; ++I) {
 NamedDecl *D = *I;
 
@@ -12497,8 +12499,7 @@
 // using decls differ if they name different scopes (but note that
 // template instantiation can cause this check to trigger when it
 // didn't before instantiation).
-if (Context.getCanonicalNestedNameSpecifier(Qual) !=
-Context.getCanonicalNestedNameSpecifier(DQual))
+if (CNNS != Context.getCanonicalNestedNameSpecifier(DQual))
   continue;
 
 Diag(NameLoc, diag::err_using_decl_redeclaration) << SS.getRange();
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6089,9 +6089,11 @@
 NNS->getAsNamespaceAlias()->getNamespace()
   ->getOriginalNamespace());
 
+  // The difference between TypeSpec and TypeSpecWithTemplate is that the
+  // latter will have the 'template' keyword when printed.
   case NestedNameSpecifier::TypeSpec:
   case NestedNameSpecifier::TypeSpecWithTemplate: {
-QualType T = getCanonicalType(QualType(NNS->getAsType(), 0));
+const Type *T = getCanonicalType(NNS->getAsType());
 
 // If we have some kind of dependent-named type (e.g., "typename T::type"),
 // break it apart into its prefix 

[PATCH] D105904: [clangd] Support `#pragma mark` in the outline

[David Goldman via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:535
+/// by range.
+std::vector mergePragmas(std::vector &Syms,
+ std::vector 
&Pragmas,

kadircet wrote:
> dgoldman wrote:
> > kadircet wrote:
> > > dgoldman wrote:
> > > > kadircet wrote:
> > > > > dgoldman wrote:
> > > > > > sammccall wrote:
> > > > > > > FWIW the flow control/how we make progress seem hard to follow 
> > > > > > > here to me.
> > > > > > > 
> > > > > > > In particular I think I'm struggling with the statefulness of "is 
> > > > > > > there an open mark group".
> > > > > > > 
> > > > > > > Possible simplifications:
> > > > > > >  - define a dummy root symbol, which seems clearer than the 
> > > > > > > vector + range
> > > > > > >  - avoid reverse-sorting the list of pragma symbols, and just 
> > > > > > > consume from the front of an ArrayRef instead
> > > > > > >  - make the outer loop over pragmas, rather than symbols. It 
> > > > > > > would first check if the pragma belongs directly here or not, and 
> > > > > > > if so, loop over symbols to work out which should become 
> > > > > > > children. This seems very likely to be efficient enough in 
> > > > > > > practice (few pragmas, or most children are grouped into pragmas)
> > > > > > > define a dummy root symbol, which seems clearer than the 
> > > > > > > vector + range
> > > > > > 
> > > > > > I guess? Then we'd take in a `DocumentSymbol & and a 
> > > > > > ArrayRef & (or just by value and then return it 
> > > > > > as well). The rest would be the same though
> > > > > > 
> > > > > > > In particular I think I'm struggling with the statefulness of "is 
> > > > > > > there an open mark group".
> > > > > > 
> > > > > > We need to track the current open group if there is one in order to 
> > > > > > move children to it.
> > > > > > 
> > > > > > > make the outer loop over pragmas, rather than symbols. It would 
> > > > > > > first check if the pragma belongs directly here or not, and if 
> > > > > > > so, loop over symbols to work out which should become children. 
> > > > > > > This seems very likely to be efficient enough in practice (few 
> > > > > > > pragmas, or most children are grouped into pragmas)
> > > > > > 
> > > > > > The important thing here is knowing where the pragma mark ends - if 
> > > > > > it doesn't, it actually gets all of the children. So we'd have to 
> > > > > > peak at the next pragma mark, add all symbols before it to us as 
> > > > > > children, and then potentially recurse to nest it inside of a 
> > > > > > symbol. I'll try it out and see if it's simpler.
> > > > > > 
> > > > > > 
> > > > > ```
> > > > > while(Pragmas) {
> > > > > // We'll figure out where the Pragmas.front() should go.
> > > > > Pragma P = Pragmas.front();
> > > > > DocumentSymbol *Cur = Root;
> > > > > while(Cur->contains(P)) {
> > > > >   auto *OldCur = Cur;
> > > > >   for(auto *C : Cur->children) {
> > > > >  // We assume at most 1 child can contain the pragma (as pragmas 
> > > > > are on a single line, and children have disjoint ranges)
> > > > >  if (C->contains(P)) {
> > > > >  Cur = C;
> > > > >  break;
> > > > >  }
> > > > >   }
> > > > >   // Cur is immediate parent of P
> > > > >   if (OldCur == Cur) {
> > > > > // Just insert P into children if it is not a group and we are 
> > > > > done.
> > > > > // Otherwise we need to figure out when current pragma is 
> > > > > terminated:
> > > > > // if next pragma is not contained in Cur, or is contained in one of 
> > > > > the children, It is at the end of Cur, nest all the children that 
> > > > > appear after P under the symbol node for P.
> > > > > // Otherwise nest all the children that appear after P but before 
> > > > > next pragma under the symbol node for P.
> > > > > // Pop Pragmas and break
> > > > >   }
> > > > > }
> > > > > }
> > > > > ```
> > > > > 
> > > > > Does that make sense, i hope i am not missing something obvious? 
> > > > > Complexity-wise in the worst case we'll go all the way down to a leaf 
> > > > > once per pragma, since there will only be a handful of pragmas most 
> > > > > of the time it shouldn't be too bad.
> > > > I've implemented your suggestion. I don't think it's simpler, but LMK, 
> > > > maybe it can be improved.
> > > oops, i was looking into an older revision and missed mergepragmas2, i 
> > > think it looks quite similar to this one but we can probably get rid of 
> > > the recursion as well and simplify a couple more cases
> > This makes sense,  I think that works for the most part besides dropping 
> > the recursion, specifically for
> > 
> > ```
> >   // Next pragma is contained in the Sym, it belongs there and doesn't
> >   // affect us at all.
> >   if (Sym.range.contains(NextPragma.DocSym.range)) {
> > Sym.children = mergePragmas2(Sym.children, Pragmas, Sym.range);
> >   

[PATCH] D107349: [Matrix] Overload stride arg in matrix.columnwise.load/store.

[Erich Keane via Phabricator via cfe-commits]

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

This looks fine to me, and seems to fix my problem, thanks!  I didn't spot 
anything obvious,and proof-read the LangRef and think it is all fine, but am 
not really the expert here, so please don't commit without the others having a 
day or two to comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107349

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

[clang] f6bc614 - [clan-format] detect function definitions more conservatively

[Krasimir Georgiev via cfe-commits]

Author: Krasimir Georgiev
Date: 2021-08-03T16:19:35+02:00
New Revision: f6bc614546e169bb1b17a29c422ebace038e6c62

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

LOG: [clan-format] detect function definitions more conservatively

https://reviews.llvm.org/D105964 updated the detection of function
definitions. It had the unfortunate effect to start marking object
definitions with attribute-like macros as function definitions.

This addresses this issue.

Reviewed By: owenpan

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

Added: 


Modified: 
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/FormatTest.cpp

Removed: 




diff  --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index 54e6c7d38e7d..de3dabcfc639 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2482,7 +2482,7 @@ static bool isFunctionDeclarationName(const FormatToken 
&Current,
   // return i + 1;
   //   }
   if (Next->Next && Next->Next->is(tok::identifier) &&
-  !(Next->MatchingParen->Next && Next->MatchingParen->Next->is(tok::semi)))
+  Line.Last->isNot(tok::semi))
 return true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {

diff  --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index c328bd44744a..de6f1b784394 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -8224,7 +8224,12 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
"f(i)\n"
"{\n"
"  return i + 1;\n"
-   "}",
+   "}\n"
+   "int\n" // Break here.
+   "f(i)\n"
+   "{\n"
+   "  return i + 1;\n"
+   "};",
Style);
   verifyFormat("int f(a, b, c);\n" // No break here.
"int\n" // Break here.
@@ -8233,8 +8238,20 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
"float c;\n"
"{\n"
"  return a + b < c;\n"
-   "}",
+   "}\n"
+   "int\n"// Break here.
+   "f(a, b, c)\n" // Break here.
+   "short a, b;\n"
+   "float c;\n"
+   "{\n"
+   "  return a + b < c;\n"
+   "};",
Style);
+  // The return breaking style doesn't affect object definitions with
+  // attribute-like macros.
+  verifyFormat("Tttt ppp\n"
+   "ABSL_GUARDED_BY(mutex) = {};",
+   getGoogleStyleWithColumns(40));
 
   Style = getGNUStyle();
 



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

[PATCH] D107269: [clan-format] detect function definitions more conservatively

[Krasimir Georgiev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf6bc614546e1: [clan-format] detect function definitions more 
conservatively (authored by krasimir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107269

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8224,7 +8224,12 @@
"f(i)\n"
"{\n"
"  return i + 1;\n"
-   "}",
+   "}\n"
+   "int\n" // Break here.
+   "f(i)\n"
+   "{\n"
+   "  return i + 1;\n"
+   "};",
Style);
   verifyFormat("int f(a, b, c);\n" // No break here.
"int\n" // Break here.
@@ -8233,8 +8238,20 @@
"float c;\n"
"{\n"
"  return a + b < c;\n"
-   "}",
+   "}\n"
+   "int\n"// Break here.
+   "f(a, b, c)\n" // Break here.
+   "short a, b;\n"
+   "float c;\n"
+   "{\n"
+   "  return a + b < c;\n"
+   "};",
Style);
+  // The return breaking style doesn't affect object definitions with
+  // attribute-like macros.
+  verifyFormat("Tttt ppp\n"
+   "ABSL_GUARDED_BY(mutex) = {};",
+   getGoogleStyleWithColumns(40));
 
   Style = getGNUStyle();
 
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2482,7 +2482,7 @@
   // return i + 1;
   //   }
   if (Next->Next && Next->Next->is(tok::identifier) &&
-  !(Next->MatchingParen->Next && Next->MatchingParen->Next->is(tok::semi)))
+  Line.Last->isNot(tok::semi))
 return true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8224,7 +8224,12 @@
"f(i)\n"
"{\n"
"  return i + 1;\n"
-   "}",
+   "}\n"
+   "int\n" // Break here.
+   "f(i)\n"
+   "{\n"
+   "  return i + 1;\n"
+   "};",
Style);
   verifyFormat("int f(a, b, c);\n" // No break here.
"int\n" // Break here.
@@ -8233,8 +8238,20 @@
"float c;\n"
"{\n"
"  return a + b < c;\n"
-   "}",
+   "}\n"
+   "int\n"// Break here.
+   "f(a, b, c)\n" // Break here.
+   "short a, b;\n"
+   "float c;\n"
+   "{\n"
+   "  return a + b < c;\n"
+   "};",
Style);
+  // The return breaking style doesn't affect object definitions with
+  // attribute-like macros.
+  verifyFormat("Tttt ppp\n"
+   "ABSL_GUARDED_BY(mutex) = {};",
+   getGoogleStyleWithColumns(40));
 
   Style = getGNUStyle();
 
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2482,7 +2482,7 @@
   // return i + 1;
   //   }
   if (Next->Next && Next->Next->is(tok::identifier) &&
-  !(Next->MatchingParen->Next && Next->MatchingParen->Next->is(tok::semi)))
+  Line.Last->isNot(tok::semi))
 return true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106343: [OpenCL] Support cl_ext_float_atomics

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added inline comments.



Comment at: clang/lib/Headers/opencl-c-base.h:24
 #define cl_khr_subgroup_clustered_reduce 1
+#define cl_ext_float_atomics
+#ifdef cl_khr_fp16

Should this be defined as `1`?

Should this define be tested in `clang/test/Headers/opencl-c-header.cl` too?


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

https://reviews.llvm.org/D106343

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

[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Manas Gupta via Phabricator via cfe-commits]

manas updated this revision to Diff 363734.
manas added a comment.

Fix test comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constant-folding.c
  clang/utils/analyzer/Dockerfile
  clang/utils/analyzer/SATestBuild.py
  clang/utils/analyzer/entrypoint.py

Index: clang/utils/analyzer/entrypoint.py
===
--- clang/utils/analyzer/entrypoint.py
+++ clang/utils/analyzer/entrypoint.py
@@ -9,10 +9,11 @@
 
 def main():
 settings, rest = parse_arguments()
+cmake_opts = ['-D' + cmd for cmd in settings.D]
 if settings.wait:
 wait()
 if settings.build_llvm or settings.build_llvm_only:
-build_llvm()
+build_llvm(cmake_opts)
 if settings.build_llvm_only:
 return
 sys.exit(test(rest))
@@ -30,14 +31,15 @@
 parser.add_argument('--wait', action='store_true')
 parser.add_argument('--build-llvm', action='store_true')
 parser.add_argument('--build-llvm-only', action='store_true')
+parser.add_argument('-D', action='append', default=[])
 return parser.parse_known_args()
 
 
-def build_llvm():
+def build_llvm(cmake_options):
 os.chdir('/build')
 try:
 if is_cmake_needed():
-cmake()
+cmake(cmake_options)
 ninja()
 except CalledProcessError:
 print("Build failed!")
@@ -55,8 +57,9 @@
 "-DCLANG_ENABLE_STATIC_ANALYZER=ON"
 
 
-def cmake():
-check_call(CMAKE_COMMAND + ' /llvm-project/llvm', shell=True)
+def cmake(cmake_options):
+check_call(CMAKE_COMMAND + ' '.join(cmake_options) + ' /llvm-project/llvm',
+shell=True)
 
 
 def ninja():
Index: clang/utils/analyzer/SATestBuild.py
===
--- clang/utils/analyzer/SATestBuild.py
+++ clang/utils/analyzer/SATestBuild.py
@@ -847,7 +847,8 @@
 continue
 
 plist = os.path.join(dir_path, filename)
-data = plistlib.readPlist(plist)
+with open(plist, "rb") as plist_file:
+data = plistlib.load(plist_file)
 path_prefix = directory
 
 if build_mode == 1:
@@ -866,7 +867,8 @@
 if 'clang_version' in data:
 data.pop('clang_version')
 
-plistlib.writePlist(data, plist)
+with open(plist, "wb") as plist_file:
+plistlib.dump(data, plist_file)
 
 
 def get_build_log_path(output_dir: str) -> str:
Index: clang/utils/analyzer/Dockerfile
===
--- clang/utils/analyzer/Dockerfile
+++ clang/utils/analyzer/Dockerfile
@@ -17,8 +17,9 @@
 gettext=0.19.8.1* \
 python3=3.6.7-1~18.04 \
 python3-pip=9.0.1-2.3* \
-cmake=3.20.5* \
-ninja-build=1.8.2-1
+cmake=3.21.1* \
+ninja-build=1.8.2-1 \
+ccache=3.4*
 
 # box2d dependencies
 RUN apt-get install -y \
Index: clang/test/Analysis/constant-folding.c
===
--- clang/test/Analysis/constant-folding.c
+++ clang/test/Analysis/constant-folding.c
@@ -281,3 +281,226 @@
 clang_analyzer_eval((b % a) < x + 10); // expected-warning{{TRUE}}
   }
 }
+
+void testAdditionRules(unsigned int a, unsigned int b, int c, int d) {
+  if (a == 0) {
+clang_analyzer_eval((a + 0) == 0); // expected-warning{{TRUE}}
+  }
+
+  // Overflows on Tmax side
+  if (a >= 5 && a <= UINT_MAX - 5 && b <= 10) {
+clang_analyzer_eval((a + b) >= UINT_MAX >> 1); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval((a + b) <= UINT_MAX >> 1); // expected-warning{{UNKNOWN}}
+  }
+
+  // Overflows on both ends
+  if (c >= INT_MIN + 5 && c <= INT_MAX - 5 && d >= -10 && d <= 10) {
+clang_analyzer_eval((c + d) <= 0); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval((c + d) >= 0); // expected-warning{{UNKNOWN}}
+  }
+
+  // Checks for unsigned operands
+  clang_analyzer_eval((a + b) < 0); // expected-warning{{FALSE}}
+  clang_analyzer_eval((a + b) <= UINT_MAX); // expected-warning{{TRUE}}
+
+  if (a == UINT_MAX && b == UINT_MAX) {
+clang_analyzer_eval((a + b) == UINT_MAX - 1); // expected-warning{{TRUE}}
+  }
+
+  // Checks for inclusive ranges for unsigned integers
+  if (a <= 10 && b <= 20) {
+clang_analyzer_eval((a + b) >= 0); // expected-warning{{TRUE}}
+clang_analyzer_eval((a + b) > 30); // expected-warning{{FALSE}}
+  }
+
+  // Checks for negative signed integers
+  if (c < 0 && d < 0) {
+clang_analyzer_eval((c + d) != -1); // expected-warning{{TRUE}}
+  }
+
+  if (c < 0 && c != INT_MIN && d < 0) {
+clang_analyzer_eval((c + d) == -1); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) == 0); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) <= -2); // expected-warning{{UNKNOWN}}
+cl

[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/utils/analyzer/Dockerfile:15-22
 RUN apt-get update && apt-get install -y \
 git=1:2.17.1* \
 gettext=0.19.8.1* \
 python3=3.6.7-1~18.04 \
 python3-pip=9.0.1-2.3* \
-cmake=3.20.5* \
-ninja-build=1.8.2-1
+cmake=3.21.1* \
+ninja-build=1.8.2-1 \

Changes to `clang/utils/analyzer/*` don't belong to this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440

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

[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Manas Gupta via Phabricator via cfe-commits]

manas added inline comments.



Comment at: clang/utils/analyzer/Dockerfile:15-22
 RUN apt-get update && apt-get install -y \
 git=1:2.17.1* \
 gettext=0.19.8.1* \
 python3=3.6.7-1~18.04 \
 python3-pip=9.0.1-2.3* \
-cmake=3.20.5* \
-ninja-build=1.8.2-1
+cmake=3.21.1* \
+ninja-build=1.8.2-1 \

vsavchenko wrote:
> Changes to `clang/utils/analyzer/*` don't belong to this patch
I messed up while `arc updating`. :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440

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

[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Manas Gupta via Phabricator via cfe-commits]

manas updated this revision to Diff 363742.
manas added a comment.

Fix unrelated commits mess up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constant-folding.c

Index: clang/test/Analysis/constant-folding.c
===
--- clang/test/Analysis/constant-folding.c
+++ clang/test/Analysis/constant-folding.c
@@ -281,3 +281,105 @@
 clang_analyzer_eval((b % a) < x + 10); // expected-warning{{TRUE}}
   }
 }
+
+void testAdditionRules(unsigned int a, unsigned int b, int c, int d) {
+  if (a == 0) {
+clang_analyzer_eval((a + 0) == 0); // expected-warning{{TRUE}}
+  }
+
+  // Overflows on Tmax side
+  if (a >= 5 && a <= UINT_MAX - 5 && b <= 10) {
+clang_analyzer_eval((a + b) >= UINT_MAX >> 1); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval((a + b) <= UINT_MAX >> 1); // expected-warning{{UNKNOWN}}
+  }
+
+  // Overflows on both ends
+  if (c >= INT_MIN + 5 && c <= INT_MAX - 5 && d >= -10 && d <= 10) {
+clang_analyzer_eval((c + d) <= 0); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval((c + d) >= 0); // expected-warning{{UNKNOWN}}
+  }
+
+  // Checks for unsigned operands
+  clang_analyzer_eval((a + b) < 0); // expected-warning{{FALSE}}
+  clang_analyzer_eval((a + b) <= UINT_MAX); // expected-warning{{TRUE}}
+
+  if (a == UINT_MAX && b == UINT_MAX) {
+clang_analyzer_eval((a + b) == UINT_MAX - 1); // expected-warning{{TRUE}}
+  }
+
+  // Checks for inclusive ranges for unsigned integers
+  if (a <= 10 && b <= 20) {
+clang_analyzer_eval((a + b) >= 0); // expected-warning{{TRUE}}
+clang_analyzer_eval((a + b) > 30); // expected-warning{{FALSE}}
+  }
+
+  // Checks for negative signed integers
+  if (c < 0 && d < 0) {
+clang_analyzer_eval((c + d) != -1); // expected-warning{{TRUE}}
+  }
+
+  if (c < 0 && c != INT_MIN && d < 0) {
+clang_analyzer_eval((c + d) == -1); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) == 0); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) <= -2); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval((c + d) >= 1); // expected-warning{{UNKNOWN}}
+  }
+
+  if (c == INT_MIN && d == INT_MIN) {
+clang_analyzer_eval((c + d) == 0); // expected-warning{{TRUE}}
+  }
+
+  if (c == INT_MIN && d < 0 && d != INT_MIN) {
+clang_analyzer_eval((c + d) > 0); // expected-warning{{TRUE}}
+  }
+
+  if (c < 0 && c >= -20 && d < 0 && d >= -40) {
+clang_analyzer_eval((c + d) < -1); // expected-warning{{TRUE}}
+clang_analyzer_eval((c + d) >= -60); // expected-warning{{TRUE}}
+  }
+
+  // Checks for integers with different sign bits
+  if (c < 0 && d > 0) {
+if (c >= -20 && d <= 10) {
+  clang_analyzer_eval((c + d) > -20); // expected-warning{{TRUE}}
+  clang_analyzer_eval((c + d) < 10); // expected-warning{{TRUE}}
+}
+  }
+
+  // Checks for overlapping signed integers ranges
+  if (c >= -20 && c <= 20 && d >= -10 && d <= 10) {
+clang_analyzer_eval((c + d) >= -30); // expected-warning{{TRUE}}
+clang_analyzer_eval((c + d) <= 30); // expected-warning{{TRUE}}
+  }
+
+  // Checks for positive signed integers
+  if (c > 0 && d > 0) {
+clang_analyzer_eval((c + d) == 1); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) == 0); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) == -1); // expected-warning{{FALSE}}
+  }
+
+  // Check when Max overflows from positive-side
+  if (c >= 10 && d >= 0 && d <= 10) {
+clang_analyzer_eval((c + d) == INT_MIN + 10); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) == -1); // expected-warning{{FALSE}}
+  }
+
+  // Checks when Min overflows from negative side
+  if (c <= 10 && d >= -10 && d <= 0) {
+clang_analyzer_eval((c + d) == 11); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) == INT_MAX - 10); // expected-warning{{FALSE}}
+  }
+
+  // Checks producing overflowing range with different signs
+  int HALF_INT_MAX = INT_MAX / 2;
+  if (c >= HALF_INT_MAX - 10 && c <= HALF_INT_MAX + 10 &&
+  d >= HALF_INT_MAX - 10 && d <= HALF_INT_MAX + 10) {
+// The resulting range for (c + d) will be:
+//   [INT_MIN, INT_MIN + 18] U [INT_MAX - 21, INT_MAX]
+clang_analyzer_eval((c + d) <= INT_MIN + 18); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval((c + d) >= INT_MAX - 21); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval((c + d) == INT_MIN + 19); // expected-warning{{FALSE}}
+clang_analyzer_eval((c + d) == INT_MAX - 22); // expected-warning{{FALSE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyze

[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Manas Gupta via Phabricator via cfe-commits]

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

I have updated the proof for Add: 
https://gist.github.com/weirdsmiley/ad6a9dbf3370e96d29f9e90068931d25


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440

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

[PATCH] D74436: Change clang option -ffp-model=precise to select ffp-contract=on

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

It looks like this patch is also causing mis-compares in both SPEC2006 and 
SPEC2017 on AArch64, e.g. in `External/SPEC/CFP2006/453.povray/`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74436

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

[PATCH] D105273: [analyzer] Introduce range-based reasoning for subtraction operator

[Manas Gupta via Phabricator via cfe-commits]

manas added a comment.

In D105273#2891921 , @manas wrote:

> Here is the proof using Z3: 
> https://gist.github.com/weirdsmiley/8a35a0e1f55f310e3566cbd47555491a

I have updated the proof for Sub.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105273

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

[PATCH] D104647: [analyzer] Support SVal::getType for pointer-to-member values

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:85
   const NamedDecl *D;
+  QualType T;
   llvm::ImmutableList L;

NoQ wrote:
> What prevents you from obtaining the type from `D`? I guess `NamedDecl` 
> itself doesn't provide a `getType()` on its own but what specific sub-classes 
> can `D` be that don't have a type?
Because it's not a declaration of `pointer-to-member`, but a declaration of the 
actual member.  It can be `nullptr` while the type is always known.
I'd prefer not to construct a pointer-to-member type myself.  It's the work for 
Sema, not for the analyzer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104647

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

[PATCH] D107325: [clang-tidy] Fix command line is too long issue which breaks test on Windows

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Hi @thakis:
I can't access the repo, could you please help me to land?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107325

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

[PATCH] D107141: [Inline-asm] Add semacheck for unsupported constraint

[Pengfei Wang via Phabricator via cfe-commits]

pengfei updated this revision to Diff 363746.
pengfei added a comment.

Implemented the handling for structure types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107141

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -313,3 +313,40 @@
   asm ("jne %l0":::);
   asm goto ("jne %l0"lab);
 }
+
+typedef struct test19_a {
+  int a;
+  char b;
+} test19_a;
+
+typedef struct test19_b {
+  int a;
+  int b;
+  int c;
+} test19_b;
+
+typedef struct test19_c {
+  char a;
+  char b;
+} test19_c;
+
+typedef struct test19_d {
+  char a;
+  char b;
+  char c;
+  char d;
+} test19_d;
+
+void test19(long long x)
+{
+  test19_a a;
+  test19_b b;
+  test19_c c;
+  test19_d d;
+  asm ("" : "=rm" (a): "0" (1)); // no-error
+  asm ("" : "=rm" (d): "0" (1)); // no-error
+  asm ("" : "=rm" (c): "0" (x)); // no-error
+  asm ("" : "=rm" (x): "0" (a)); // no-error
+  asm ("" : "=rm" (a): "0" (d)); // no-error
+  asm ("" : "=rm" (b): "0" (1)); // expected-error {{unsupported inline asm: input with type 'int' matching output with type 'test19_b' (aka 'struct test19_b')}}
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -617,14 +617,32 @@
   AD_Int, AD_FP, AD_Other
 } InputDomain, OutputDomain;
 
-if (InTy->isIntegerType() || InTy->isPointerType())
+auto IsPower2Structure = [this](QualType Ty) {
+  if (!Ty->isStructureType())
+return false;
+
+  switch (Context.getTypeSize(Ty)) {
+  default:
+return false;
+  case 8:
+  case 16:
+  case 32:
+  case 64:
+  case 128:
+return true;
+  }
+};
+
+if (InTy->isIntegerType() || InTy->isPointerType() ||
+IsPower2Structure(InTy))
   InputDomain = AD_Int;
 else if (InTy->isRealFloatingType())
   InputDomain = AD_FP;
 else
   InputDomain = AD_Other;
 
-if (OutTy->isIntegerType() || OutTy->isPointerType())
+if (OutTy->isIntegerType() || OutTy->isPointerType() ||
+IsPower2Structure(OutTy))
   OutputDomain = AD_Int;
 else if (OutTy->isRealFloatingType())
   OutputDomain = AD_FP;
@@ -666,7 +684,8 @@
 // output was a register, just extend the shorter one to the size of the
 // larger one.
 if (!SmallerValueMentioned && InputDomain != AD_Other &&
-OutputConstraintInfos[TiedTo].allowsRegister())
+OutputConstraintInfos[TiedTo].allowsRegister() &&
+OutputDomain != AD_Other)
   continue;
 
 // Either both of the operands were mentioned or the smaller one was
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -2467,22 +2467,40 @@
 // that is usually cheaper, but LLVM IR should really get an anyext someday.
 if (Info.hasTiedOperand()) {
   unsigned Output = Info.getTiedOperand();
-  QualType OutputType = S.getOutputExpr(Output)->getType();
+  const Expr *OutputExpr = S.getOutputExpr(Output);
+  QualType OutputType = OutputExpr->getType();
   QualType InputTy = InputExpr->getType();
+  uint64_t OutputSize = getContext().getTypeSize(OutputType);
+  uint64_t InputSize = getContext().getTypeSize(InputTy);
 
-  if (getContext().getTypeSize(OutputType) >
-  getContext().getTypeSize(InputTy)) {
+  if (OutputSize > InputSize) {
 // Use ptrtoint as appropriate so that we can do our extension.
 if (isa(Arg->getType()))
   Arg = Builder.CreatePtrToInt(Arg, IntPtrTy);
+if (isa(Arg->getType())) {
+  llvm::IntegerType *IntTy =
+  llvm::IntegerType::get(getLLVMContext(), InputSize);
+  Arg = Builder.CreateBitCast(Arg, IntTy);
+}
 llvm::Type *OutputTy = ConvertType(OutputType);
 if (isa(OutputTy))
   Arg = Builder.CreateZExt(Arg, OutputTy);
 else if (isa(OutputTy))
   Arg = Builder.CreateZExt(Arg, IntPtrTy);
-else {
-  assert(OutputTy->isFloatingPointTy() && "Unexpected output type");
+else if (OutputTy->isFloatingPointTy())
   Arg = Builder.CreateFPExt(Arg, OutputTy);
+else {
+  assert(OutputTy->isStructTy() && "Unexpected output type");
+  if (OutputSize == 128)
+// TODO: We may need to extract the first element from structure
+// and just assign the value to it when the OutputSize == 128.
+CGM.getDiags().Report(S.getAsmLoc(), diag::err_asm_invalid_type)
+<< OutputExpr->getType() << 1 /*Output*/
+<< OutputExpr->getSourceRange(

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D106394#2921830 , @kimgr wrote:

> Took me a while to get my head around it, but I see now that this is only 
> supported for system headers. I think that makes sense for the compiler, 
> otherwise it will be hard to guess which headers are allowed to include what. 
> IWYU usually doesn't have that problem, as we analyze source files 
> individually, and usually not headers independently.

Not sure I follow - could you describe IWYU's approach in more detail? 
Certainly a clang warning can be powered by examining a source file and all the 
includes in totality, not only using local reasoning at the location of one 
#include from another file. So if there's a way to use more global (within the 
scope of a single translation unit) reasoning to get a more general purpose 
feature, that would be an option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106394

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

[PATCH] D106614: [Clang] add btf_tag attribute

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added inline comments.



Comment at: clang/test/Sema/attr-btf_tag.c:31
+
+int __tag2 foo(struct t1 *arg); // expected-warning {{attribute 
'btf_tag("tag2")' ignored as not in later redeclaration or definition}}
+int __tag foo(struct t1 *arg);

aaron.ballman wrote:
> yonghong-song wrote:
> > aaron.ballman wrote:
> > > This looks backwards to me -- I think the initial declaration of `foo()` 
> > > is fine and shouldn't be diagnosed, it's the subsequent declarations of 
> > > `foo()` with a different `btf_tag` argument that should be diagnosed.
> > > 
> > > I think additional test cases for these semantics is:
> > > ```
> > > void bar();
> > > void __tag bar(); // (I think I understood that you want this to 
> > > diagnose.)
> > > 
> > > void __tag bar();
> > > void bar(); // (But that you don't want this to diagnose.)
> > > ```
> > > 
> > There are a little complication here. Maybe you can give some advice.
> > We could have code like
> >D1: void __tag1 __tag2 bar();
> >D2: void bar();
> > The reason is that we explicitly allow multiple btf_tag's in the same 
> > declaration and this is exactly our use case.
> > 
> > Current merge*Attribute() framework will take one attribute at a time.
> > For example, first we will do
> > mergeBTFTagAttribute(D2, __tag1):
> >   Here, we add __tag1 to D2, so we have
> >D2: void __tag1 bar();
> > we then do
> > mergeBTFTagAttribute(D2, __tag2):
> >   but D2 already has a __tag1 which is incompatible with __tag2, and
> >   we will issue an error here. But it is incorrect to issue an error
> >   since the correct semantics is to inherit both __tag1 and __tag2 for 
> > D2.
> > 
> > Let me take a look at the code how to best handle such cases. Please also 
> > let me know if you have any suggestions. thanks!
> > 
> > A little bit more. The following are possible cases to check:
> >(1)  void bar();
> >  void __tag1 __tag2 bar();  // fail
> >(2).  void __tag1 bar();
> >  void __tag1 __tag2 bar(); // fail
> >(3)   void __tag1 __tag2 bar();
> >  void __tag1 __tag2 bar();  // succeed
> >(4). void __tag1 __tag2 __tag3 bar();
> >  void __tag1 __tag2 bar();   // fail
> >(5). void __tag3 bar();
> >  void __tag1 __tag2 bar(); // fail
> >(6). void __tag1 __tag2 bar();
> >  void bar()  // succeed
> > 
> > Basically, for two same declaration except attributes, D1, D2,
> >no error/warning only if D2 has no btf_tag attributes, or
> >D1 and D2 have identical btf_tag attributes.
> > Do this sound reasonable?
> > Let me take a look at the code how to best handle such cases. Please also 
> > let me know if you have any suggestions. thanks!
> 
> Oh, that's... interesting! You want to accept multiple, potentially separate 
> attribute specifiers on the initial declaration, but not allow any new 
> attributes on redeclarations or the definition. Btw, there are some tests 
> missing from your above comment. You also need to think about:
> ```
> void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
> void bar(); // succeed
> 
> void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
> void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // succeed
> 
> void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
> void __attribute__((btf_tag("one"))) bar(); // fail
> 
> void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
> void __attribute__((btf_tag("one"), btf_tag("two"), btf_tag("three"))) bar(); 
> // fail
> 
> [[clang::btf_tag("one")]] void bar [[clang::btf_tag("two")]] (); // ok
> void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // succeed
> ```
> I don't think we have any attributes that behave like that currently. Can you 
> remind me: what is the concern if the attributes are additive on 
> redeclaration?
>> Can you remind me: what is the concern if the attributes are additive on 
>> redeclaration?

I guess my main concern is people may look at one place for attribute 
availability. They may find it not there but actually it is defined in a 
different places. But in practice this may not be an issue as people typically 
will put attributes in the header *unique* declaration and/or the later 
*unique* definition.

Let me just to use additive approach which will simplify the whole thing a lot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614

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

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D107292#2921901 , @aaron.ballman 
wrote:

> In D107292#2920774 , @dblaikie 
> wrote:
>
>> In D107292#2920746 , @cjdb wrote:
>>
>>> In D107292#2920637 , @dblaikie 
>>> wrote:
>>>
 Not a huge objection - but minor quandry: What's the motivation for this 
 patch? I don't know of any codebase that encourages/uses the alternative 
 tokens and I wonder if adding more usability to them is a worthwhile 
 investment in clang's codebase complexity, etc.
>>>
>>> There are codebases that use them (all of my non-Google, non-LLVM code 
>>> does, for example, and I'm not the sole user: just a loud one who's also in 
>>> a position to patch tooling).
>>
>> Ah, any pointers to large open source projects that use this?
>
> https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=bitand&search=Search
>
> (Searching for 'and' is a bit less useful because of how much it shows up in 
> assembly, comments, etc.)

Ah, cool. Only case I could find there (that wasn't C code or compiler test 
cases) was something called FuzzyLite (which looks like it hasn't been touched 
in 4 years or so).

I don't fundamentally object to this now it's being proposed as a clang-tidy 
thing - bar should be low/easy for experiments, getting user experience, 
adoption, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107292

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