[PATCH] D113738: [LTO] Allow passing -Os/-Oz as the optimization level

[Vincent Lee via Phabricator via cfe-commits]

thevinster added inline comments.



Comment at: lld/Common/Args.cpp:29
 
+OptimizationLevel args::getOptLevel(llvm::opt::InputArgList &args,
+unsigned int key,

nit: `getOptLevel` seems a bit too generic for something that's LTO specific. 
The MachO port has the concept of passing an opt level that generates more 
efficient opcodes. Suggestions are welcome here - perhaps `getLTOOptLevel` 
would be better here? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113738

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

[PATCH] D113558: [clang-tidy] Fix cppcoreguidelines-virtual-base-class-destructor in macros

[Whisperity via Phabricator via cfe-commits]

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

The fix is concise, thank you for observing and untangling the crash!

As the check was originally introduced in the ongoing release, we can go 
without a release notes entry.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:288
+protected:
+  virtual CONCAT(~Foo, Bar2()); // no-fixit
+};

It is strange that there is no fix-it here even though the keyword appears as a 
single token ...[1]



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:288
+protected:
+  virtual CONCAT(~Foo, Bar2()); // no-fixit
+};

whisperity wrote:
> It is strange that there is no fix-it here even though the keyword appears as 
> a single token ...[1]
Maybe a FIXME could be added for this case, just to register that we indeed 
realised something is //strange// here, but I'm not convinced neither for nor 
against. The purpose of the patch is to get rid of the crash, after all.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:299
+protected:
+  CONCAT(vir, tual) ~FooBar3();
+};

[1]... whereas here the check generates the fixit which removes the entire 
macro substitution!


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

https://reviews.llvm.org/D113558

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

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D64454#3124423 , @carlosgalvezp 
wrote:

> I was a bit confused as well about the ClangSA support in clang-tidy. What's 
> the current status? Is clang-tidy running *all* checks from StaticAnalyzer?

If your package is built with CSA enabled, and you don't want to run //Cross 
Translation Unit// analysis mode, then yeah. After all, both Clang-Tidy and 
Clang Static Analyser are just "FrontendAction" libraries under the hood.



In D64454#3124312 , @aaron.ballman 
wrote:

> Yeah, I was worried this would get out of sycn and it really did not take 
> long for that to happen in practice. I think there's utility in listing the 
> checks in clang-tidy's documentation instead of pointing users to the SA 
> documentation page. Users should have one website to go to where they can see 
> what clang-tidy checks are available to them.

Does this still apply given that the quality and formatting match of the 
documentation website increased over the past one or two releases? The old 
website  if I see correctly no longer lists 
the checkers (and the previous format was also increasingly outdated and bad 
experience...), and we have the RST-based new website 
.

Come to think of it, maybe some RST hacks could automate "pulling in" the list 
from the (new) CSA website to the Tidy list, as long as maybe they are in a 
separate category (heading) in the Tidy list?
Although I'm unsure how much being `/tools/clang/` and 
`/tools/clang/tools/extra/` mess this thing up.
In case of Python, for example, there is a well-understood way of registering 
the documentation site of third-party packages, and //your// documentation 
generator can automatically generate the cross-reference that links to the 
other, from your doc's point-of-view, external website. But that involves a 
considerable amount of "magic".
As far as I know, Sphinx is written in Python, so one could write Python code 
that runs //during// the documentation generation (at least the RST part, I'm 
not sure about the `man` or `infopages` or `groff` stuff!) that can do "magic".



What we //COULD// do, however, is get rid of the individual checker 
documentation files (which all just contain machine generated redirects, at 
least according to this diff...) and have ALL the cross-referencing links 
(hand-written, autogenerated by our integration, or autogenerated by RST magic) 
point to not a page living inside Tidy's scope that just does a redirect, but 
to the appropriate heading in the (new) CSA doc suite?

(Then again, irrespective of what magic we will use, the non-trivial grouping 
structure on the new CSA docs site can become a problem...)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64454

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

[PATCH] D113863: [clang-tidy] Make `readability-container-data-pointer` more robust

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:20-23
+static const char ContainerExprName[] = "container-expr";
+static const char DerefContainerExprName[] = "deref-container-expr";
+static const char AddrofContainerExprName[] = "addrof-container-expr";
+static const char AddressOfName[] = "address-of";

Perhaps we could use `constexpr llvm::StringLiteral`?


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

https://reviews.llvm.org/D113863

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

This introduces another way of setting (optional) architecture extensions and 
having two ways to do the same is nearly always a bad thing, which is how one 
of my colleagues phrased it.

This was already a complex area and thus I don't think introducing another is 
going to really simplify things. This is the main problem of this proposal. 
Other things are:

- ideally we keep these flags consistent with GCC, but that seems to be a no-go.
- how do these new `-m` flags and `-march` interact?

More subjective: for most users this whole `-march` business is abstracted away 
in build systems, so they won't have to deal with this, that's why this isn't 
so much of an improvement.

If we want a better user experience set options, there are probably other 
things that are more important, like checking legal/illegal architecture 
combinations.

So, in summary, we prefer not to go ahead with this. And the precedent that was 
mentioned, `-mcrc`, should probably be deprecated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113848: [clang-tidy][NFC] Refactor ClangTidyDiagnosticConsumer files

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

What is the motivation behind the change? Can we be sure that these classes 
that were moved out weren't details to the context itself, and are 
understandable, usable, and can be relied upon without the context object?



In D113848#3130542 , @carlosgalvezp 
wrote:

> I agree with the comments, but I didn't want to touch any code other than 
> moving things around, since it's hard to see the changes in the diff 
> otherwise. I was also unsure if this was "LLVM convention" or a mistake. I'm 
> happy to fix in a separate patch if that's OK?

I think you can fix it now, LLVM style doesn't indent stuff that's within a 
namespace, so it won't change the meaningful part of the diff visually.




Comment at: clang-tools-extra/clang-tidy/ClangTidyContext.cpp:40-41
+#include 
+using namespace clang;
+using namespace tidy;
+

Eugene.Zelenko wrote:
> Shouldn't namespaces be used instead?
In general, you'd also want to make it more explicit by saying 

```
using namespace clang;
using namespace clang::tidy;
```

and not rely on precisely one `tidy` being visible from the previous set of 
available declarations.



Comment at: clang-tools-extra/clang-tidy/ClangTidyError.h:35-36
+
+} // end namespace tidy
+} // end namespace clang
+

Is this formatted properly? I thought the appropriate closing comment is `// 
namespace blah`, without the `end`. 😲 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113848

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};

carlosgalvezp wrote:
> salman-javed-nz wrote:
> > What's the prevalent style for class member initialization? `=` or `{}`?
> > cppcoreguidelines-prefer-member-initializer defaults to `{}` but I have 
> > seen both types in the code.
> I tried to find this info in the LLVM coding guidelines but didn't find 
> anything, so I assume it's maybe up to developers' discretion.
> 
> I prefer using braced initialization, since it prevents implicit conversions:
> https://godbolt.org/z/4sP4rGsrY
> 
> More strict guidelines like Autosar enforce this. Also CppCoreGuidelines 
> prefer that style as you point out.
I think this is such a new and within LLVM relatively unused feature (remember 
we are still pegged to C++14...) that we do not have a consensus on style, and 
perhaps warrants discussing it on the mailing list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113847

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};

whisperity wrote:
> carlosgalvezp wrote:
> > salman-javed-nz wrote:
> > > What's the prevalent style for class member initialization? `=` or `{}`?
> > > cppcoreguidelines-prefer-member-initializer defaults to `{}` but I have 
> > > seen both types in the code.
> > I tried to find this info in the LLVM coding guidelines but didn't find 
> > anything, so I assume it's maybe up to developers' discretion.
> > 
> > I prefer using braced initialization, since it prevents implicit 
> > conversions:
> > https://godbolt.org/z/4sP4rGsrY
> > 
> > More strict guidelines like Autosar enforce this. Also CppCoreGuidelines 
> > prefer that style as you point out.
> I think this is such a new and within LLVM relatively unused feature 
> (remember we are still pegged to C++14...) that we do not have a consensus on 
> style, and perhaps warrants discussing it on the mailing list.
Just to clarify - this is C++11/14 syntax, nothing really fancy. I'm happy to 
change to " = 0" if people want, I don't have a strong opinion here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113847

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

[PATCH] D113848: [clang-tidy][NFC] Refactor ClangTidyDiagnosticConsumer files

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> What is the motivation behind the change?

The motivation is cleaning the file to make it easier to find things. As a n00b 
to clang-tidy I find it a bit hard to navigate the code and find the different 
classes - they are not where I expect them to be. The `ClangTidyContext` is a 
core class used in other places than just `DiagnosticConsumer`, so I think it 
makes sense to keep it on it's own file so it's easy to find. For example:

  ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context);

It's unintuitive to have to import the `DiagnosticConsumer` to get the `Context`

In general I think it's a good principle to keep a 1:1 mapping between public 
classes and files. Naturally, internal helper classes can be kept in the same 
file as the "main" class. This brings build-time improvements as well but I'd 
say they are negligible.

Let me know if you agree, otherwise I can of course drop this :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113848

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D113779#3130701 , @SjoerdMeijer 
wrote:

> This introduces another way of setting (optional) architecture extensions and 
> having two ways to do the same is nearly always a bad thing, which is how one 
> of my colleagues phrased it.

I think there's a subtle difference to using the `-mXXX` options vs `-march`. 
`-mXXX` adds a feature *without* changing the default architecture version. 
Unless I am missing something, it's not possible to selectively add target 
features using `-march` without also forcing the architecture version to some 
particular version.

AFAICT the only alternative to `-mXXX` options would be using `-Xclang 
-target-feature -Xclang +dotprod`, which is very verbose and not documented.

The `-mXXX` flags would allow users to conveniently write forward-compatible 
compiler invocations that also ensure a minimum set of features is available. 
Using `-mXXX` flags is very common on X86 and providing a similar interface for 
AArch64 will likely help users transitioning.

> - how do these new `-m` flags and `-march` interact?

The `-mXXX` flags add the target feature to the list of target features, the 
same as additional features provided to `-march`. It should be equivalent to 
specifying extra flags via `-march` in terms of checking for invalid 
combinations and conflict resolution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113776: [Clang][SVE] Properly enable/disable dependant SVE target features based upon +(no)sve.* options

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: llvm/unittests/Support/TargetParserTest.cpp:908
+ AArch64::AEK_I8MM | AArch64::AEK_SVE |
+ AArch64::AEK_SVE2 | AArch64::AEK_SVE2BITPERM |
  AArch64::AEK_PAUTH | AArch64::AEK_MTE |

Has AEK_SVE2 been doubled up in this constant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113776

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added subscribers: manojgupta, DavidSpickett.
DavidSpickett added a comment.

There was a similar proposal for crypto https://reviews.llvm.org/D60472.

Quoting @manojgupta for the pitch for that:

> The motivation for this change is to make "crypto" setting an additive option 
> e.g. like "-mavx" used in many media packages. Some packages in Chrome want 
> to enable crypto conditionally for a few files to allow crypto feature to be 
> used based on runtime cpu detection. They set "-march=armv8+crypto" flag but 
> it gets overridden by the global "-march=armv8a" flag set by the build system 
> in Chrome OS because the target cpu does not support crypto causing 
> compile-time errors.
> Ability to specify "-mcrypto" standalone makes it an additive option and 
> ensures that it it is not lost. i.e. this will help in decoupling "-mcrypto" 
> from "-march" so that they could be set independently. The current additive 
> alternate is '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.

Concerns were raised there.

Seems like we're balancing adding a bunch of (clang only for at least the short 
term) options versus making the build system(s) cope with having to track what 
the last `-march` was.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113118: [clang][AST] Check context of record in structural equivalence.

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

balazske added a comment.

In the last case the the RecordDecl's `a` and `b` can be matched (not internal 
statements in a function) and this is similar to the existing test cases. The 
`struct a` and `struct b` already fails to match because different name.




Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1360-1364
+// Special case: We allow a struct defined in a function to be equivalent
+// with a similar struct defined outside of a function.
+if ((DC1->isFunctionOrMethod() && DC2->isTranslationUnit()) ||
+(DC2->isFunctionOrMethod() && DC1->isTranslationUnit()))
+  return true;

steakhal wrote:
> Shouldn't you also check the name as well?
Here `DC1` or `DC2` is already a function or translation unit, the name of it 
is not relevant.



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1379
+
+DC1 = DC1->getParent()->getNonTransparentContext();
+DC2 = DC2->getParent()->getNonTransparentContext();

steakhal wrote:
> So, the previous `DC1->isTranslationUnit()` guards the `DC1->getParent()` to 
> never be `NULL`; same for `DC2`.
> Pretty neat.
Change to `!DC1->getParent()` (from `DC1->isTranslationUnit()`)? (The line can 
be moved to here to increase readability, so the break statement is at end of 
the loop.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113118

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

(I should note that crytpo isn't the best example because it means different 
things to different base architectures, that part doesn't apply here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D113779#3130744 , @DavidSpickett 
wrote:

> There was a similar proposal for crypto https://reviews.llvm.org/D60472.
>
> Quoting @manojgupta for the pitch for that:
>
>> The motivation for this change is to make "crypto" setting an additive 
>> option e.g. like "-mavx" used in many media packages. Some packages in 
>> Chrome want to enable crypto conditionally for a few files to allow crypto 
>> feature to be used based on runtime cpu detection. They set 
>> "-march=armv8+crypto" flag but it gets overridden by the global 
>> "-march=armv8a" flag set by the build system in Chrome OS because the target 
>> cpu does not support crypto causing compile-time errors.
>> Ability to specify "-mcrypto" standalone makes it an additive option and 
>> ensures that it it is not lost. i.e. this will help in decoupling "-mcrypto" 
>> from "-march" so that they could be set independently. The current additive 
>> alternate is '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.
>
> Concerns were raised there.
>
> Seems like we're balancing adding a bunch of (clang only for at least the 
> short term) options versus making the build system(s) cope with having to 
> track what the last `-march` was.

Thanks for sharing the related patch/discussion.

> versus making the build system(s) cope with having to track what the last 
> `-march` was.

Please see my earlier response. There can be use cases where the build system 
may not know what the last `-march` was, because the user may not have 
specified an explicit `-march` (e.g. they want to use the default picked by the 
compiler). Is there a way for the build system to conveniently determine what 
the default architecture version of the compiler is?

Also pushing this to the build system vendors means we need to convince X 
build-system vendors to adjust their implementations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113201: [clang-tidy] Fix a crash in modernize-loop-convert around conversion operators

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp:155-167
+/// Look through conversion/copy constructors and operators to find the 
explicit
 /// initialization expression, returning it is found.
 ///
 /// The main idea is that given
 ///   vector v;
 /// we consider either of these initializations
 ///   vector::iterator it = v.begin();

steakhal wrote:
> I think it's called //conversion member functions//.
Perhaps it would be nice if an example was added to the comment here which 
involves said conversion.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp:278
+  // conversion operator, which has no name, to Q::const_iterator.
+  Q Qt;
+  for (Q::const_iterator It = Qt.begin(), E = Qt.end(); It != E; ++It) {

This is not specific to //Qt//, the GUI framework, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113201

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

[PATCH] D113148: Add new clang-tidy check for string_view(nullptr)

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D113148#3109190 , @CJ-Johnson 
wrote:

> In D113148#3108993 , @aaron.ballman 
> wrote:
>
>> Generally speaking, we prefer to improve the existing checks. I think 
>> `bugprone-string-constructor` would probably be a better place for the 
>> constructor-related functionality. But that still means we don't have a good 
>> place for things like the assignment and comparison functionality, and it 
>> seems more useful to keep all of the `string_view`-with-`nullptr` logic in 
>> one place. That said, we should be careful we're not too onerous when users 
>> enable all `bugprone` checks together.
>
> As for "we should be careful we're not too onerous when users enable all 
> `bugprone` checks together.", these fixes are about preventing UB. While I 
> did put this in the "bugprone" module, perhaps "prone" is the wrong way to 
> think about it. It's unconditionally UB in all cases, not just a potential 
> bug. The suggested fixes are important for preventing crashes in the short 
> term, but more importantly for allowing the code to build in the future, 
> since the constructor overload `basic_string_view(nullptr_t) = delete;` is 
> being added.

@aaron.ballman Maybe we should do something to ease the burden of `bugprone-` 
becoming a catch-all basket, and create a new checker group for the 
"unconditional UB" or "in almost all cases this will make you do bad things" 
kinds of stuff.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:82-90
+  Checks for various ways that the ``nullptr`` literal can be passed into the
+  ``const CharT*`` constructor of ``std::basic_string_view`` and replaces them
+  with the default constructor in most cases. For the comparison operators,
+  braced initializer list does not compile so instead the empty string is used.
+  Also, ``==`` and ``!=`` are replaced with calls to ``.empty()``.
+
+  This prevents code from invoking behavior which is unconditionally undefined.

Usually we only put a one sentence short summary into the release notes.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:13-14
+This prevents code from invoking behavior which is unconditionally undefined.
+The single-argument ``const char*`` constructor of ``std::string_view`` does
+not check for the null case before dereferencing its input.
+

You mentioned in the review discussion that a constructor taking 
`std::nullptr_t` will be added with an explicit `= delete;`. Maybe it is worth 
mentioning it here, if we have a standard version or a proposal on track for 
changing the STL like that?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:16-17
+
+.. code-block:: c++
+  std::string_view sv = nullptr;
+

Will this work? Isn't the `:` making RST confused here? You usually need an 
empty line because the block "instance" may be given additional options in RST.

It might compile the docs still, so this is the kind of thing that must be 
verified visually.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:46
+
+Note: The source pattern with trailing comment "A" selects the
+``(const CharT*)`` constructor overload and then value-initializes the pointer,

There should be a `.. note::` block which renders a nice box of note with an 
icon.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:49
+causing a null dereference. It happens to not include the ``nullptr`` literal,
+but it is still within the scope of this ClangTidy check.
+

Clang-Tidy. Or just simply "this check".



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:53
+``(const CharT*, size_type)`` constructor which is perfectly valid, since the
+length argument is ``0``. It is not changed by this ClangTidy check.

ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148

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

[PATCH] D111100: enable plugins for clang-tidy

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Wait... Is the diff //really// supposed to be this small? You didn't include 
code... that one header takes care of **everything**?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D00

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

[PATCH] D113118: [clang][AST] Check context of record in structural equivalence.

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

balazske updated this revision to Diff 387181.
balazske added a comment.

Updated tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113118

Files:
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -929,6 +929,138 @@
   EXPECT_TRUE(testStructuralMatch(First, Second));
 }
 
+struct StructuralEquivalenceRecordContextTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNoVsNamed) {
+  auto Decls = makeDecls("class X;", "namespace N { class X; }",
+Lang_CXX03, cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNamedVsNamed) {
+  auto Decls = makeDecls("namespace A { class X; }",
+"namespace B { class X; }", Lang_CXX03,
+cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceAnonVsNamed) {
+  auto Decls = makeDecls("namespace { class X; }",
+"namespace N { class X; }", Lang_CXX03,
+cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNoVsAnon) {
+  auto Decls = makeDecls("class X;", "namespace { class X; }",
+Lang_CXX03, cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceAnonVsAnon) {
+  auto Decls = makeDecls("namespace { class X; }",
+"namespace { class X; }", Lang_CXX03,
+cxxRecordDecl());
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceAnonVsAnonAnon) {
+  auto Decls = makeDecls("namespace { class X; }",
+"namespace { namespace { class X; } }",
+Lang_CXX03, cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest,
+   NamespaceNamedNamedVsNamedNamed) {
+  auto Decls = makeDecls(
+  "namespace A { namespace N { class X; } }",
+  "namespace B { namespace N { class X; } }", Lang_CXX03, cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNamedVsInline) {
+  auto Decls = makeDecls(
+  "namespace A { namespace A { class X; } }",
+  "namespace A { inline namespace A { class X; } }", Lang_CXX17,
+  cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceInlineVsInline) {
+  auto Decls = makeDecls(
+  "namespace A { inline namespace A { class X; } }",
+  "namespace A { inline namespace B { class X; } }", Lang_CXX17,
+  cxxRecordDecl());
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceInlineTopLevel) {
+  auto Decls = makeDecls(
+  "inline namespace A { class X; } }",
+  "inline namespace B { class X; } }", Lang_CXX17,
+  cxxRecordDecl());
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, TransparentContext) {
+  auto Decls = makeDecls("extern \"C\" { class X; }", "class X;",
+Lang_CXX03, cxxRecordDecl());
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, TransparentContextNE) {
+  auto Decls = makeDecls("extern \"C\" { class X; }",
+"namespace { class X; }", Lang_CXX03,
+cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, TransparentContextInNamespace) {
+  auto Decls = makeDecls("extern \"C\" { namespace N { class X; } }",
+"namespace N { extern \"C\" { class X; } }",
+Lang_CXX03, cxxRecordDecl());
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, NamespaceOfRecordMember) {
+  auto Decls = makeDecls(
+  R"(
+  class X;
+  class Y { X* x; };
+  )",
+  R"(
+  namespace N { class X; }
+  class Y { N::X* x; };
+  )",
+  Lang_CXX03, cxxRecordDecl(hasName("Y")));
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, StructDefini

[PATCH] D113752: [Parse] Use empty RecoveryExpr when if/while/do/switch conditions fail to parse

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

this looks great, some nits and questions.




Comment at: clang/lib/Parse/ParseStmt.cpp:1194
SourceLocation Loc,
-   Sema::ConditionKind CK,
+   Sema::ConditionKind CK, bool MissingOK,
SourceLocation *LParenLoc,

what's the purpose of introducing `MissingOK`? It seems unclear to me, my 
understanding is

- before the patch, ActionOnCondition always returned a valid ConditionResult 
for an empty ConditionResult. I assume this was conceptually a "MissingOK-true" 
case.
- now in the patch, the MissingOK is propagated to `ActOnCondition`, which 
determines the returning result;  ​The MissingOK is set to false in 
ParseSwitchStatement, ParseDoStatement below. 

The only case I can think of is that we might fail to create a recoveryExpr 
(when the recovery-ast flag is off) for the condition.



Comment at: clang/lib/Parse/ParseStmt.cpp:1811
 
-  if (Cond.isInvalid() || Body.isInvalid())
+  if (Body.isInvalid())
 return StmtError();

we should keep the `Cond.isInvalid()` branch, because the above typo-correction 
code path (Line1800) can hit it.



Comment at: clang/lib/Sema/SemaExpr.cpp:19149
   if (!SubExpr)
-return ConditionResult();
+return ConditionResult(/*Invalid=*/!MissingOK);
 

nit: `return MissingOK? ConditionResult() : ConditionError()`, is easier to 
understand.



Comment at: clang/lib/Sema/SemaExpr.cpp:19149
   if (!SubExpr)
-return ConditionResult();
 

hokein wrote:
> nit: `return MissingOK? ConditionResult() : ConditionError()`, is easier to 
> understand.
while here, we always return a valid condition (conceptually MissingOK is 
true), but in the patch, the default value of `MissingOK` parameter is false, 
this seems we are changing the default behavior for all call sides of 
`ActOnCondition`.



Comment at: clang/test/AST/loop-recovery.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s

file name: loop-recovery.cpp => ast-dump-loop-recovery.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113752

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Paul Walker via Phabricator via cfe-commits]

paulwalker-arm added a comment.

Rather than adding connivence options after the fact what about allowing 
`-march=` to be specified multiple times? The first must be the usual format 
with later ones required to start with `+`.  The defined parsing behaviour 
would be as if there was a single `-march` instance positioned at the first 
occurrence but containing the value of all instances when combined from left to 
right.  For example `-march=armv8.4-a .. march=+nofp16` or perhaps `+=` 
syntax like  `-march=armv8.4-a .. march+=nofp16+nosve` is more intuitive?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1w.c:235
+// CHECK-NEXT:ret void
+//
 void test_svst1w_scatter_u64base_index_u64(svbool_t pg, svuint64_t bases, 
int64_t index, svuint64_t data)

Unfortunately it looks like these were not autogenerated, they used to use 
CHECK-DAG. This results in more changes than we expect. My thinking is that 
these tests are intended to be autogenerated, so we should make a clean patch 
which autogenerates the tests and then reapply your work on top of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489

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

[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

It looks like some auto-generation lines were added in 
91eda9c30f33da6ec6da70b59a5f5da6c6397039 
, but the 
tests using CHECK-DAG were not actually auto-generated. It seems we could 
either cleanly auto-generate them, or we should copy the existing CHECK-DAG 
patterns. It seems that what is currently committed is a little wrong, if only 
by having a 'these tests are auto-generated' header even though they are not 
auto-generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489

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

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Kristina Bessonova via Phabricator via cfe-commits]

krisb abandoned this revision.
krisb added a comment.

Abandon in favor of https://reviews.llvm.org/D113741.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

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

[PATCH] D113558: [clang-tidy] Fix cppcoreguidelines-virtual-base-class-destructor in macros

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 387189.
steakhal marked 2 inline comments as done.
steakhal added a comment.

Add fixme comment for:

  class FooBar2 {
  protected:
virtual CONCAT(~Foo, Bar2()); // FIXME: We should have a fixit for this.
  };


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

https://reviews.llvm.org/D113558

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -270,3 +270,53 @@
 DerivedFromTemplateNonVirtualBaseStruct2Typedef 
InstantiationWithPublicNonVirtualBaseStruct2;
 
 } // namespace Bugzilla_51912
+
+namespace macro_tests {
+#define CONCAT(x, y) x##y
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar1' is 
protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar1 {
+protected:
+  CONCAT(vir, tual) CONCAT(~Foo, Bar1()); // no-fixit
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar2' is 
protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar2 {
+protected:
+  virtual CONCAT(~Foo, Bar2()); // FIXME: We should have a fixit for this.
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:7: warning: destructor of 'FooBar3' is 
protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:7: note: make it protected and non-virtual
+// CHECK-FIXES:  class FooBar3 {
+// CHECK-FIXES-NEXT: protected:
+// CHECK-FIXES-NEXT:   ~FooBar3();
+// CHECK-FIXES-NEXT: };
+class FooBar3 {
+protected:
+  CONCAT(vir, tual) ~FooBar3();
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:7: warning: destructor of 'FooBar4' is 
protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:7: note: make it protected and non-virtual
+// CHECK-FIXES:  class FooBar4 {
+// CHECK-FIXES-NEXT: protected:
+// CHECK-FIXES-NEXT:   ~CONCAT(Foo, Bar4());
+// CHECK-FIXES-NEXT: };
+class FooBar4 {
+protected:
+  CONCAT(vir, tual) ~CONCAT(Foo, Bar4());
+};
+
+// CHECK-MESSAGES: :[[@LINE+3]]:7: warning: destructor of 'FooBar5' is 
protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+2]]:7: note: make it protected and non-virtual
+#define XMACRO(COLUMN1, COLUMN2) COLUMN1 COLUMN2
+class FooBar5 {
+protected:
+  XMACRO(CONCAT(vir, tual), ~CONCAT(Foo, Bar5());) // no-crash, no-fixit
+};
+#undef XMACRO
+#undef CONCAT
+} // namespace macro_tests
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -46,9 +46,12 @@
   this);
 }
 
-static CharSourceRange
+static Optional
 getVirtualKeywordRange(const CXXDestructorDecl &Destructor,
const SourceManager &SM, const LangOptions &LangOpts) {
+  if (Destructor.getLocation().isMacroID())
+return None;
+
   SourceLocation VirtualBeginLoc = Destructor.getBeginLoc();
   SourceLocation VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
   Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
@@ -190,8 +193,10 @@
   Fix = FixItHint::CreateInsertion(Destructor->getLocation(), "virtual ");
 } else if (Destructor->getAccess() == AccessSpecifier::AS_protected) {
   ProtectedAndVirtual = true;
-  Fix = FixItHint::CreateRemoval(getVirtualKeywordRange(
-  *Destructor, *Result.SourceManager, Result.Context->getLangOpts()));
+  if (const auto MaybeRange =
+  getVirtualKeywordRange(*Destructor, *Result.SourceManager,
+ Result.Context->getLangOpts()))
+Fix = FixItHint::CreateRemoval(*MaybeRange);
 }
   } else {
 Fix = generateUserDeclaredDestructor(*MatchedClassOrStruct,


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -270,3 +270,53 @@
 DerivedFromTemplateNonVirtualBaseStruct2Typedef InstantiationWithPublicNonVirtualBaseStruct2;
 
 } // namespace Bugzilla_51912
+

[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

My current thinking is that the route forward is to update the tests without 
update test checks, which affects only the st1{b,h,w} files, and possibly to 
remove the false header implying these files were autogenerated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489

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

[clang] 9fec50f - [cmake] use project relative paths when generating ASTNodeAPI.json

[Matheus Izvekov via cfe-commits]

Author: Matheus Izvekov
Date: 2021-11-15T12:35:34+01:00
New Revision: 9fec50f001b1ab86ff36c6e92336ae1c1d3fcdef

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

LOG: [cmake] use project relative paths when generating ASTNodeAPI.json

Signed-off-by: Matheus Izvekov 

Reviewed By: stephenneuendorffer

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

Added: 


Modified: 
clang/lib/Tooling/CMakeLists.txt

Removed: 




diff  --git a/clang/lib/Tooling/CMakeLists.txt 
b/clang/lib/Tooling/CMakeLists.txt
index 558385b0eb5a..403d2dfb45e8 100644
--- a/clang/lib/Tooling/CMakeLists.txt
+++ b/clang/lib/Tooling/CMakeLists.txt
@@ -60,11 +60,11 @@ else()
   $
 # Skip this in debug mode because parsing AST.h is too slow
 --skip-processing=${skip_expensive_processing}
--I ${CMAKE_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
--I ${CMAKE_SOURCE_DIR}/../clang/include
--I ${CMAKE_BINARY_DIR}/tools/clang/include
--I ${CMAKE_BINARY_DIR}/include
--I ${CMAKE_SOURCE_DIR}/include
+-I ${LLVM_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
+-I ${CLANG_SOURCE_DIR}/include
+-I ${LLVM_BINARY_DIR}/tools/clang/include
+-I ${LLVM_BINARY_DIR}/include
+-I ${LLVM_SOURCE_DIR}/include
 ${implicitDirs}
 --json-output-path ${CMAKE_CURRENT_BINARY_DIR}/ASTNodeAPI.json
   )



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

[PATCH] D113664: [cmake] use project relative paths when generating ASTNodeAPI.json

[Matheus Izvekov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9fec50f001b1: [cmake] use project relative paths when 
generating ASTNodeAPI.json (authored by mizvekov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113664

Files:
  clang/lib/Tooling/CMakeLists.txt


Index: clang/lib/Tooling/CMakeLists.txt
===
--- clang/lib/Tooling/CMakeLists.txt
+++ clang/lib/Tooling/CMakeLists.txt
@@ -60,11 +60,11 @@
   $
 # Skip this in debug mode because parsing AST.h is too slow
 --skip-processing=${skip_expensive_processing}
--I ${CMAKE_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
--I ${CMAKE_SOURCE_DIR}/../clang/include
--I ${CMAKE_BINARY_DIR}/tools/clang/include
--I ${CMAKE_BINARY_DIR}/include
--I ${CMAKE_SOURCE_DIR}/include
+-I ${LLVM_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
+-I ${CLANG_SOURCE_DIR}/include
+-I ${LLVM_BINARY_DIR}/tools/clang/include
+-I ${LLVM_BINARY_DIR}/include
+-I ${LLVM_SOURCE_DIR}/include
 ${implicitDirs}
 --json-output-path ${CMAKE_CURRENT_BINARY_DIR}/ASTNodeAPI.json
   )


Index: clang/lib/Tooling/CMakeLists.txt
===
--- clang/lib/Tooling/CMakeLists.txt
+++ clang/lib/Tooling/CMakeLists.txt
@@ -60,11 +60,11 @@
   $
 # Skip this in debug mode because parsing AST.h is too slow
 --skip-processing=${skip_expensive_processing}
--I ${CMAKE_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
--I ${CMAKE_SOURCE_DIR}/../clang/include
--I ${CMAKE_BINARY_DIR}/tools/clang/include
--I ${CMAKE_BINARY_DIR}/include
--I ${CMAKE_SOURCE_DIR}/include
+-I ${LLVM_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
+-I ${CLANG_SOURCE_DIR}/include
+-I ${LLVM_BINARY_DIR}/tools/clang/include
+-I ${LLVM_BINARY_DIR}/include
+-I ${LLVM_SOURCE_DIR}/include
 ${implicitDirs}
 --json-output-path ${CMAKE_CURRENT_BINARY_DIR}/ASTNodeAPI.json
   )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113880: [clang][modules] Infer framework modules in explicit builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith, vsapsai.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When the dependency scanner discovers dependency on an inferred module, it 
correctly reports the original modulemap path (instead of the virtual 
`__inferred_module.map` file) as the input file for building the module.

However, during explicit build, `-fimplicit-module-maps` is necessary to 
successfully build such module. Without the flag, the module is not inferred:

  // HeaderSearch.cpp:1681
case LMM_InvalidModuleMap:
  // Try to infer a module map from the framework directory.
  if (HSOpts->ImplicitModuleMaps)
ModMap.inferFrameworkModule(Dir, IsSystem, /*Parent=*/nullptr);
  break;

... and the build fails with "unknown module" error.

To support explicit builds of inferred modules, D102495 
 stopped removing the 
`-fimplicit-module-maps` from the command lines produced by dependency scanner.

While this works, implicit module maps don't seem like a good fit for explicit 
modules: since the module map necessary to build a module is already known, we 
don't need to discover `.modulemap` files in search paths.

Besides the possibility of performing unnecessary filesystem operations, 
`-fimplicit-module-maps` can uncover different semantics between explicit and 
implicit modules: D113775 .

This patch tries attempts to infer modules even in explicit builds if possible, 
which also enables the dependency scanner to always disable the implicit module 
maps feature.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113880

Files:
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/Modules/explicit-build-inferred.cpp

Index: clang/test/Modules/explicit-build-inferred.cpp
===
--- clang/test/Modules/explicit-build-inferred.cpp
+++ clang/test/Modules/explicit-build-inferred.cpp
@@ -1,11 +1,10 @@
 // RUN: rm -rf %t && mkdir %t
 //
-// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fimplicit-module-maps \
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules \
 // RUN:   -emit-module -x c++ %S/Inputs/explicit-build-inferred/frameworks/module.modulemap \
 // RUN:   -fmodule-name=Inferred -o %t/Inferred.pcm -F %S/Inputs/explicit-build-inferred/frameworks
 //
 // RUN: %clang_cc1 -fmodules -fno-implicit-modules -fsyntax-only %s \
-// RUN:   -fmodule-map-file=%S/Inputs/explicit-build-inferred/frameworks/module.modulemap \
 // RUN:   -fmodule-file=%t/Inferred.pcm -F %S/Inputs/explicit-build-inferred/frameworks
 
 #include 
Index: clang/test/ClangScanDeps/modules-pch.c
===
--- clang/test/ClangScanDeps/modules-pch.c
+++ clang/test/ClangScanDeps/modules-pch.c
@@ -22,6 +22,7 @@
 // CHECK-PCH-NEXT: "-cc1"
 // CHECK-PCH:  "-emit-module"
 // CHECK-PCH:  "-fmodules"
+// CHECK-PCH-NOT:  "-fimplicit-module-maps"
 // CHECK-PCH:  "-fmodule-name=ModCommon1"
 // CHECK-PCH:  "-fno-implicit-modules"
 // CHECK-PCH:],
@@ -39,6 +40,7 @@
 // CHECK-PCH-NEXT: "-cc1"
 // CHECK-PCH:  "-emit-module"
 // CHECK-PCH:  "-fmodules"
+// CHECK-PCH-NOT:  "-fimplicit-module-maps"
 // CHECK-PCH:  "-fmodule-name=ModCommon2"
 // CHECK-PCH:  "-fno-implicit-modules"
 // CHECK-PCH:],
@@ -63,6 +65,7 @@
 // CHECK-PCH:  "-emit-module"
 // CHECK-PCH:  "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_COMMON_2]]/ModCommon2-{{.*}}.pcm"
 // CHECK-PCH:  "-fmodules"
+// CHECK-PCH-NOT:  "-fimplicit-module-maps"
 // CHECK-PCH:  "-fmodule-name=ModPCH"
 // CHECK-PCH:  "-fno-implicit-modules"
 // CHECK-PCH:],
@@ -140,6 +143,7 @@
 // CHECK-TU-NEXT:   "command-line": [
 // CHECK-TU-NEXT: "-cc1",
 // CHECK-TU:  "-emit-module",
+// CHECK-TU-NOT:  "-fimplicit-module-maps",
 // CHECK-TU:  "-fmodule-name=ModTU",
 // CHECK-TU:  "-fno-implicit-modules",
 // CHECK-TU:],
@@ -206,6 +210,7 @@
 // CHECK-TU-WITH-COMMON:  "-fmodule-map-file=[[PREFIX]]/module.modulemap"
 // CHECK-TU-WITH-COMMON:  "-emit-module",
 // CHECK-TU-WITH-COMMON:  "-fmodule-file=[[PREFIX]]/build/{{.*}}/ModCommon1-{{.*}}.pcm",
+// CHECK-TU-WITH-COMMON-NOT:  "-fimplicit-module-maps",
 // CHECK-TU-WITH-COMMON:  "-fmodule-name=ModTUWithCommon",
 // CHECK-TU-WITH-COMMON:  "-fno-implicit-mo

[PATCH] D113775: [clang][modules] Umbrella with missing submodule: unify implicit & explicit

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/test/Modules/missing-submodule.m:3
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F 
%S/Inputs %s -verify
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -F 
%S/Inputs %s -verify
 #include  // expected-warning{{missing submodule 
'Module.NotInModule'}}

vsapsai wrote:
> Haven't checked the implementation but does the test cover any new behavior? 
> Based on description it should test explicit modules. But the added line is 
> testing `-fimplicit-module-maps`, just using the system-wide shared modules 
> cache.
The system-wide shared modules cache flag is being generated by the driver. 
Here I'm invoking `-cc1` without `-fmodules-cache-path=`, which I think 
effectively disables implicit modules. I'll add `-fno-implicit-modules` to make 
this more obvious. So yes, this test does cover the new behavior.

However, I spent some time investigating why we're using 
`-fimplicit-module-maps` in the command lines for explicit build produced by 
the dependency scanner and came to the conclusion this is due to inferred 
modules. I tried to fix the underlying issue in D113880. If we don't pass 
`-fimplicit-module-maps` to Clang, we don't hit the case this patch solves. I'm 
not sure whether I should abandon this patch in favor of D113880, or commit it 
anyway, since it still might be useful for some.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113775

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

[PATCH] D113828: [clang-tidy] Fix false positives in `fuchsia-trailing-return` check involving deduction guides

[Julie Hockett via Phabricator via cfe-commits]

juliehockett resigned from this revision.
juliehockett added a comment.

Apologies, i's been a good long while since I was active in this area. @phosek 
is the most likely to know who would be the best contact in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113828

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

[PATCH] D113775: [clang][modules] Umbrella with missing submodule: unify implicit & explicit

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 387199.
jansvoboda11 added a comment.

Add `-fno-implicit-modules` to test case to make it more obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113775

Files:
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Modules/missing-submodule.m

Index: clang/test/Modules/missing-submodule.m
===
--- clang/test/Modules/missing-submodule.m
+++ clang/test/Modules/missing-submodule.m
@@ -1,5 +1,6 @@
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs %s -verify
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fno-implicit-modules   -F %S/Inputs %s -verify
 #include  // expected-warning{{missing submodule 'Module.NotInModule'}}
 
 int getNotInModule() {
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1772,9 +1772,24 @@
   return MS_ModuleNotFound;
 }
 
+static ModuleLoadResult handleMissingSubmodule(DiagnosticsEngine &Diags,
+   Module *SubM,
+   SourceLocation ImportLoc,
+   ModuleIdPath Path) {
+  // We have an umbrella header or directory that doesn't actually include all
+  // the headers within the directory it covers. Complain about this missing
+  // submodule and recover by forgetting that we ever saw this submodule.
+  Diags.Report(ImportLoc, diag::warn_missing_submodule)
+  << SubM->getFullModuleName()
+  << SourceRange(Path.front().second, Path.back().second);
+
+  return ModuleLoadResult::MissingExpected;
+}
+
 ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
 StringRef ModuleName, SourceLocation ImportLoc,
-SourceLocation ModuleNameLoc, bool IsInclusionDirective) {
+SourceLocation ModuleNameLoc, ModuleIdPath Path,
+Module::NameVisibilityKind Visibility, bool IsInclusionDirective) {
   // Search for a module with the given name.
   HeaderSearch &HS = PP->getHeaderSearchInfo();
   Module *M =
@@ -1797,6 +1812,16 @@
   return ModuleLoadResult::ConfigMismatch;
 }
 
+bool MapPrivateSubModToTopLevel = false;
+// FIXME: Should we detect this at module load time? It seems fairly
+// expensive (and rare).
+Module *SubM = getSubmoduleCorrespondingToPath(
+MapPrivateSubModToTopLevel, M, ImportLoc, Path, Visibility,
+IsInclusionDirective);
+
+if (SubM != M && !SubM->IsFromModuleFile && !MapPrivateSubModToTopLevel)
+  return handleMissingSubmodule(getDiagnostics(), SubM, ImportLoc, Path);
+
 getDiagnostics().Report(ModuleNameLoc, diag::err_module_build_disabled)
 << ModuleName;
 return nullptr;
@@ -2061,8 +2086,9 @@
 //}
 MM.cacheModuleLoad(*Path[0].first, Module);
   } else {
-ModuleLoadResult Result = findOrCompileModuleAndReadAST(
-ModuleName, ImportLoc, ModuleNameLoc, IsInclusionDirective);
+ModuleLoadResult Result =
+findOrCompileModuleAndReadAST(ModuleName, ImportLoc, ModuleNameLoc,
+  Path, Visibility, IsInclusionDirective);
 if (!Result.isNormal())
   return Result;
 if (!Result)
@@ -2077,6 +2103,9 @@
 return ModuleLoadResult();
 
   bool MapPrivateSubModToTopLevel = false;
+  class Module *TopLevelModule = Module;
+  // FIXME: Should we detect this at module load time? It seems fairly
+  // expensive (and rare).
   Module = getSubmoduleCorrespondingToPath(MapPrivateSubModToTopLevel, Module,
ImportLoc, Path, Visibility,
IsInclusionDirective);
@@ -2084,19 +2113,9 @@
   // Make the named module visible, if it's not already part of the module
   // we are parsing.
   if (ModuleName != getLangOpts().CurrentModule) {
-if (!Module->IsFromModuleFile && !MapPrivateSubModToTopLevel) {
-  // We have an umbrella header or directory that doesn't actually include
-  // all of the headers within the directory it covers. Complain about
-  // this missing submodule and recover by forgetting that we ever saw
-  // this submodule.
-  // FIXME: Should we detect this at module load time? It seems fairly
-  // expensive (and rare).
-  getDiagnostics().Report(ImportLoc, diag::warn_missing_submodule)
-<< Module->getFullModuleName()
-<< SourceRange(Path.front().second, Path.back().second);
-
-  return ModuleLoadResult::MissingExpected;
-}
+if (Module != TopLevelModule && !Module->IsFromModuleFile &&
+!MapPrivateSubModToTopLevel)
+  return handleMissingSubmodule(getDiagnostics(), Module, ImportLo

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Kind ping @aaron.ballman @whisperity


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113422

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

[PATCH] D113844: [clang-format] [NFC] build clang-format with -Wall

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D113844#3130186 , 
@HazardyKnusperkeks wrote:

> Is okay, but I think the compiler is a bit overreacting, since there is only 
> one call (chain).

I agree, but I think I saw that one of the buildbot was running with -Wall and 
I got dinged on my commit and this was our only failure (tests use include ../ 
which is also a failure)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113844

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

[clang-tools-extra] 29a8d45 - [clang-tidy] Fix a crash in modernize-loop-convert around conversion operators

[Kirstóf Umann via cfe-commits]

Author: Kristóf Umann
Date: 2021-11-15T13:11:29+01:00
New Revision: 29a8d45c5a239c9fa6b8634a9de3d655064816d1

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

LOG: [clang-tidy] Fix a crash in modernize-loop-convert around conversion 
operators

modernize-loop-convert checks and fixes when a loop that iterates over the
elements of a container can be rewritten from a for(...; ...; ...) style into
the "new" C++11 for-range format. For that, it needs to parse the elements of
that loop, like its init-statement, such as ItType it = cont.begin().
modernize-loop-convert checks whether the loop variable is initialized by a
begin() member function.

When an iterator is initialized with a conversion operator (e.g. for
(const_iterator it = non_const_container.begin(); ...), attempts to retrieve the
name of the initializer expression resulted in an assert, as conversion
operators don't have a valid IdentifierInfo.

I fixed this by making digThroughConstructors dig through conversion operators
as well.

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

Added: 


Modified: 
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h

clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp 
b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index e78f96d1f0c2..432d929057d1 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -304,8 +304,8 @@ StatementMatcher makePseudoArrayLoopMatcher() {
 static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin,
 bool *IsArrow, bool IsReverse) 
{
   // FIXME: Maybe allow declaration/initialization outside of the for loop.
-  const auto *TheCall =
-  dyn_cast_or_null(digThroughConstructors(Init));
+  const auto *TheCall = dyn_cast_or_null(
+  digThroughConstructorsConversions(Init));
   if (!TheCall || TheCall->getNumArgs() != 0)
 return nullptr;
 

diff  --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp 
b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
index 74dd4a3047ea..35fe51f11cd8 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
@@ -152,20 +152,21 @@ bool DeclFinderASTVisitor::VisitTypeLoc(TypeLoc TL) {
   return true;
 }
 
-/// Look through conversion/copy constructors to find the explicit
-/// initialization expression, returning it is found.
+/// Look through conversion/copy constructors and member functions to find the
+/// explicit initialization expression, returning it is found.
 ///
 /// The main idea is that given
 ///   vector v;
 /// we consider either of these initializations
 ///   vector::iterator it = v.begin();
 ///   vector::iterator it(v.begin());
+///   vector::const_iterator it(v.begin());
 /// and retrieve `v.begin()` as the expression used to initialize `it` but do
 /// not include
 ///   vector::iterator it;
 ///   vector::iterator it(v.begin(), 0); // if this constructor existed
 /// as being initialized from `v.begin()`
-const Expr *digThroughConstructors(const Expr *E) {
+const Expr *digThroughConstructorsConversions(const Expr *E) {
   if (!E)
 return nullptr;
   E = E->IgnoreImplicit();
@@ -178,8 +179,13 @@ const Expr *digThroughConstructors(const Expr *E) {
 E = ConstructExpr->getArg(0);
 if (const auto *Temp = dyn_cast(E))
   E = Temp->getSubExpr();
-return digThroughConstructors(E);
+return digThroughConstructorsConversions(E);
   }
+  // If this is a conversion (as iterators commonly convert into their const
+  // iterator counterparts), dig through that as well.
+  if (const auto *ME = dyn_cast(E))
+if (const auto *D = dyn_cast(ME->getMethodDecl()))
+  return 
digThroughConstructorsConversions(ME->getImplicitObjectArgument());
   return E;
 }
 
@@ -357,7 +363,7 @@ static bool isAliasDecl(ASTContext *Context, const Decl 
*TheDecl,
   bool OnlyCasts = true;
   const Expr *Init = VDecl->getInit()->IgnoreParenImpCasts();
   if (isa_and_nonnull(Init)) {
-Init = digThroughConstructors(Init);
+Init = digThroughConstructorsConversions(Init);
 OnlyCasts = false;
   }
   if (!Init)

diff  --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h 
b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
index 7dd2c8ef107f..8b288efd1b04 100644
--- a/clan

[PATCH] D113201: [clang-tidy] Fix a crash in modernize-loop-convert around conversion operators

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Szelethus marked 2 inline comments as done.
Closed by commit rG29a8d45c5a23: [clang-tidy] Fix a crash in 
modernize-loop-convert around conversion operators (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D113201?vs=385459&id=387206#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113201

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
  clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp
@@ -273,6 +273,16 @@
   // CHECK-FIXES: for (int & It : Tt)
   // CHECK-FIXES-NEXT: printf("I found %d\n", It);
 
+  // Do not crash because of Qq.begin() converting. Q::iterator converts with a
+  // conversion operator, which has no name, to Q::const_iterator.
+  Q Qq;
+  for (Q::const_iterator It = Qq.begin(), E = Qq.end(); It != E; ++It) {
+printf("I found %d\n", *It);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : Qq)
+  // CHECK-FIXES-NEXT: printf("I found %d\n", It);
+
   T *Pt;
   for (T::iterator It = Pt->begin(), E = Pt->end(); It != E; ++It) {
 printf("I found %d\n", *It);
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
===
--- clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
@@ -53,6 +53,23 @@
   iterator end();
 };
 
+struct Q {
+  typedef int value_type;
+  struct const_iterator {
+value_type &operator*();
+const value_type &operator*() const;
+const_iterator &operator++();
+bool operator!=(const const_iterator &other);
+void insert(value_type);
+value_type X;
+  };
+  struct iterator {
+operator const_iterator() const;
+  };
+  iterator begin();
+  iterator end();
+};
+
 struct U {
   struct iterator {
 Val& operator*();
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
===
--- clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
@@ -275,7 +275,7 @@
 typedef llvm::SmallVector UsageResult;
 
 // General functions used by ForLoopIndexUseVisitor and LoopConvertCheck.
-const Expr *digThroughConstructors(const Expr *E);
+const Expr *digThroughConstructorsConversions(const Expr *E);
 bool areSameExpr(ASTContext *Context, const Expr *First, const Expr *Second);
 const DeclRefExpr *getDeclRef(const Expr *E);
 bool areSameVariable(const ValueDecl *First, const ValueDecl *Second);
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
===
--- clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
@@ -152,20 +152,21 @@
   return true;
 }
 
-/// Look through conversion/copy constructors to find the explicit
-/// initialization expression, returning it is found.
+/// Look through conversion/copy constructors and member functions to find the
+/// explicit initialization expression, returning it is found.
 ///
 /// The main idea is that given
 ///   vector v;
 /// we consider either of these initializations
 ///   vector::iterator it = v.begin();
 ///   vector::iterator it(v.begin());
+///   vector::const_iterator it(v.begin());
 /// and retrieve `v.begin()` as the expression used to initialize `it` but do
 /// not include
 ///   vector::iterator it;
 ///   vector::iterator it(v.begin(), 0); // if this constructor existed
 /// as being initialized from `v.begin()`
-const Expr *digThroughConstructors(const Expr *E) {
+const Expr *digThroughConstructorsConversions(const Expr *E) {
   if (!E)
 return nullptr;
   E = E->IgnoreImplicit();
@@ -178,8 +179,13 @@
 E = ConstructExpr->getArg(0);
 if (const auto *Temp = dyn_cast(E))
   E = Temp->getSubExpr();
-return digThroughConstructors(E);
+return digThroughConstructorsConversions(E);
   }
+  // If this is a conversion (as iterators commonly convert into their const
+  // iterator counterparts), dig through that as well.
+  if (const auto *ME = dyn_cast(E))
+if (const auto *D = d

[PATCH] D113776: [Clang][SVE] Properly enable/disable dependant SVE target features based upon +(no)sve.* options

[Bradley Smith via Phabricator via cfe-commits]

bsmith updated this revision to Diff 387212.
bsmith edited the summary of this revision.
bsmith added a comment.
Herald added a subscriber: kristof.beyls.

- Fix duplicate arch feature in unit test
- Use enum class instead of plain enum with typedef


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113776

Files:
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/test/Driver/aarch64-cpus.c
  clang/test/Driver/aarch64-implied-sve-features.c
  llvm/include/llvm/Support/AArch64TargetParser.def
  llvm/unittests/Support/TargetParserTest.cpp

Index: llvm/unittests/Support/TargetParserTest.cpp
===
--- llvm/unittests/Support/TargetParserTest.cpp
+++ llvm/unittests/Support/TargetParserTest.cpp
@@ -903,11 +903,11 @@
  AArch64::AEK_SIMD | AArch64::AEK_RAS |
  AArch64::AEK_LSE | AArch64::AEK_RDM |
  AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
- AArch64::AEK_SVE2 | AArch64::AEK_BF16 |
- AArch64::AEK_I8MM | AArch64::AEK_SVE2BITPERM |
- AArch64::AEK_PAUTH | AArch64::AEK_MTE |
- AArch64::AEK_SSBS | AArch64::AEK_FP16FML |
- AArch64::AEK_SB,
+ AArch64::AEK_BF16 | AArch64::AEK_I8MM |
+ AArch64::AEK_SVE | AArch64::AEK_SVE2 |
+ AArch64::AEK_SVE2BITPERM | AArch64::AEK_PAUTH |
+ AArch64::AEK_MTE | AArch64::AEK_SSBS |
+ AArch64::AEK_FP16FML | AArch64::AEK_SB,
  "9-A"),
 ARMCPUTestParams("cortex-a57", "armv8-a", "crypto-neon-fp-armv8",
  AArch64::AEK_CRC | AArch64::AEK_CRYPTO |
@@ -1012,12 +1012,12 @@
  AArch64::AEK_CRC | AArch64::AEK_FP |
  AArch64::AEK_SIMD | AArch64::AEK_RAS |
  AArch64::AEK_LSE | AArch64::AEK_RDM |
- AArch64::AEK_RCPC | AArch64::AEK_SVE2 |
- AArch64::AEK_DOTPROD | AArch64::AEK_MTE |
- AArch64::AEK_PAUTH | AArch64::AEK_I8MM |
- AArch64::AEK_BF16 | AArch64::AEK_SVE2BITPERM |
- AArch64::AEK_SSBS | AArch64::AEK_SB |
- AArch64::AEK_FP16FML,
+ AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
+ AArch64::AEK_MTE | AArch64::AEK_PAUTH |
+ AArch64::AEK_I8MM | AArch64::AEK_BF16 |
+ AArch64::AEK_SVE | AArch64::AEK_SVE2 |
+ AArch64::AEK_SVE2BITPERM | AArch64::AEK_SSBS |
+ AArch64::AEK_SB | AArch64::AEK_FP16FML,
  "9-A"),
 ARMCPUTestParams("cyclone", "armv8-a", "crypto-neon-fp-armv8",
  AArch64::AEK_NONE | AArch64::AEK_CRYPTO |
Index: llvm/include/llvm/Support/AArch64TargetParser.def
===
--- llvm/include/llvm/Support/AArch64TargetParser.def
+++ llvm/include/llvm/Support/AArch64TargetParser.def
@@ -145,9 +145,10 @@
 AARCH64_CPU_NAME("cortex-a55", ARMV8_2A, FK_CRYPTO_NEON_FP_ARMV8, false,
  (AArch64::AEK_FP16 | AArch64::AEK_DOTPROD | AArch64::AEK_RCPC))
 AARCH64_CPU_NAME("cortex-a510", ARMV9A, FK_NEON_FP_ARMV8, false,
- (AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE2BITPERM |
+ (AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SB |
   AArch64::AEK_PAUTH | AArch64::AEK_MTE | AArch64::AEK_SSBS |
-  AArch64::AEK_SB | AArch64::AEK_FP16FML))
+  AArch64::AEK_SVE | AArch64::AEK_SVE2 | AArch64::AEK_SVE2BITPERM |
+  AArch64::AEK_FP16FML))
 AARCH64_CPU_NAME("cortex-a57", ARMV8A, FK_CRYPTO_NEON_FP_ARMV8, false,
  (AArch64::AEK_CRC))
 AARCH64_CPU_NAME("cortex-a65", ARMV8_2A, FK_CRYPTO_NEON_FP_ARMV8, false,
@@ -184,8 +185,9 @@
   AArch64::AEK_SSBS))
 AARCH64_CPU_NAME("cortex-x2", ARMV9A, FK_NEON_FP_ARMV8, false,
  (AArch64::AEK_MTE | AArch64::AEK_BF16 | AArch64::AEK_I8MM |
-  AArch64::AEK_PAUTH | AArch64::AEK_SSBS | AArch64::AEK_SVE2BITPERM |
-  AArch64::AEK_SB | AArch64::AEK_FP16FML))
+  AArch64::AEK_PAUTH | AArch64::AEK_SSBS | AArch64::AEK_SB |
+  AArch64::AEK_SVE | AArch64::AEK_SVE2 | AArch64::AEK_SVE2BITPERM |
+  AArch64::AEK_FP16FML))
 AARCH64_CPU_NAME("neoverse-e1", ARMV8_2A, FK_CRYPTO_NEON_FP_ARMV8, false,
  (AArch64::AEK_DOTPROD | AArch64::AEK_FP16 | AArch64::AEK_RAS |
 

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/GlobList.h:50-52
+/// A \p GlobList that caches search results, so that search is performed only
+/// once for the same query.
+class CachedGlobList {

If `CachedGlobList` //is-a// `GlobList`, wouldn't it be better to express the 
inheritance relationship?



Comment at: clang-tools-extra/clang-tidy/GlobList.h:61
+private:
+  GlobList Globs;
+  enum Tristate { None, Yes, No };

(considering the logically first data member is the conceptual base class)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113422

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

[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[Paul Walker via Phabricator via cfe-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

There's an issue with the a `Value*` being named `VecPtrTy` but otherwise this 
looks good to me.

I'll leave it up to you to decide whether it's worth breaking out the usage of 
update_cc_test_checks.py into a separate patch.  Normally this is a good thing 
but given the patch is ready to go and I imagine you wouldn't submit the "use 
update_cc_test_checks.py" patch for review I'm not sure it's worth the effort.




Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:736
+  Type *VecTy = II.getType();
+  Value *VecPtrTy = Builder.CreateBitCast(PtrOp, VecTy->getPointerTo());
+

The name here is wrong as this is not a type.  My guess is you meant `VecPtr`?



Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:758
+  Value *PtrOp = II.getOperand(2);
+  Value *VecPtrTy =
+  Builder.CreateBitCast(PtrOp, VecOp->getType()->getPointerTo());

As above, I suspect this name is not what you intended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489

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

[PATCH] D113804: [clang-tidy] Fix behavior of `modernize-use-using` with nested structs/unions

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h:27
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  std::map LastTagDeclRanges;
+

Considering the pointer is just a number that hashes well, is there a reason to 
use the plain `std::map`? It is [[ 
https://llvm.org/docs/ProgrammersManual.html#map | generally discouraged ]].



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp:305-308
+
+typedef struct { int a; union { int b; }; } PR50990;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990 = struct { int a; union { int b; }; };

Just for the sake of testing a new logic, it would be beneficial to add a "more 
complex" nested example. At least one example where there are two sibling 
nests, and one where there is an additional level of nesting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113804

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Btw, I'm in WG14 (C) standards meetings all week this week and on vacation next 
week, so my availability for code reviews is pretty limited for the next while. 
So despite my comments here, if you don't get a response back from me in a 
timely manner, don't hold up the review on my account if someone else approves 
(I have no blocking concerns with this patch).




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};

carlosgalvezp wrote:
> whisperity wrote:
> > carlosgalvezp wrote:
> > > salman-javed-nz wrote:
> > > > What's the prevalent style for class member initialization? `=` or `{}`?
> > > > cppcoreguidelines-prefer-member-initializer defaults to `{}` but I have 
> > > > seen both types in the code.
> > > I tried to find this info in the LLVM coding guidelines but didn't find 
> > > anything, so I assume it's maybe up to developers' discretion.
> > > 
> > > I prefer using braced initialization, since it prevents implicit 
> > > conversions:
> > > https://godbolt.org/z/4sP4rGsrY
> > > 
> > > More strict guidelines like Autosar enforce this. Also CppCoreGuidelines 
> > > prefer that style as you point out.
> > I think this is such a new and within LLVM relatively unused feature 
> > (remember we are still pegged to C++14...) that we do not have a consensus 
> > on style, and perhaps warrants discussing it on the mailing list.
> Just to clarify - this is C++11/14 syntax, nothing really fancy. I'm happy to 
> change to " = 0" if people want, I don't have a strong opinion here.
I've seen both styles but I believe I've seen `=` used more often for scalar 
initialization and `{}` used more often for ctor initialization. e.g.,
```
int a = 0; // More often
int a{0}; // Less often

ClassFoo c = {1, 2, "three"}; // Less often
ClassFoo c{1, 2, "three"}; // More often
```



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:61-64
-  unsigned errorsIgnored() const {
+  int errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }

The type changes overflow here from well-defined behavior to undefined 
behavior. I don't think that's a serious concern (we have error limits 
already), but in general, I don't think we should be changing types like this 
without stronger rationale. This particular bit feels like churn without 
benefit -- what do we gain by introducing the possibility for UB here? (I'm not 
strongly opposed, but I get worried when well-defined code turns into 
potentially undefined code in the name of an NFC cleanup.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113847

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

[PATCH] D113148: Add new clang-tidy check for string_view(nullptr)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D113148#3130779 , @whisperity 
wrote:

> In D113148#3109190 , @CJ-Johnson 
> wrote:
>
>> In D113148#3108993 , 
>> @aaron.ballman wrote:
>>
>>> Generally speaking, we prefer to improve the existing checks. I think 
>>> `bugprone-string-constructor` would probably be a better place for the 
>>> constructor-related functionality. But that still means we don't have a 
>>> good place for things like the assignment and comparison functionality, and 
>>> it seems more useful to keep all of the `string_view`-with-`nullptr` logic 
>>> in one place. That said, we should be careful we're not too onerous when 
>>> users enable all `bugprone` checks together.
>>
>> As for "we should be careful we're not too onerous when users enable all 
>> `bugprone` checks together.", these fixes are about preventing UB. While I 
>> did put this in the "bugprone" module, perhaps "prone" is the wrong way to 
>> think about it. It's unconditionally UB in all cases, not just a potential 
>> bug. The suggested fixes are important for preventing crashes in the short 
>> term, but more importantly for allowing the code to build in the future, 
>> since the constructor overload `basic_string_view(nullptr_t) = delete;` is 
>> being added.
>
> @aaron.ballman Maybe we should do something to ease the burden of `bugprone-` 
> becoming a catch-all basket, and create a new checker group for the 
> "unconditional UB" or "in almost all cases this will make you do bad things" 
> kinds of stuff.

`bugprone` was originally added because `misc` was becoming too much of a 
catch-all basket and we wanted a new group for the "doing this is a bug" kind 
of checks, so there's precedent for splitting groups when we need finer 
granularity. However, we had quite a few checks that could be moved from `misc` 
to `bugrone` during that switch. If we added a group for unconditional UB (or 
morally similar), it'd be good to know how many checks we think need to move 
and what sort of checks we envision the new module encouraging in the future. 
That'll help inform us whether it's good value to make a split or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148

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

[PATCH] D112143: [X86][ABI] Do not return float/double from x87 registers when x87 is disabled

[Phoebe Wang via Phabricator via cfe-commits]

pengfei added inline comments.



Comment at: clang/test/Driver/x86-target-features.c:5
 // RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mno-80387 %s -### 
-o %t.o 2>&1 | FileCheck -check-prefix=NO-X87 %s
+// RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mno-fp-ret-in-387 
%s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-X87 %s
 // X87: "-target-feature" "+x87"

nickdesaulniers wrote:
> If you split this test and the above change to 
> clang/include/clang/Driver/Options.td into a separate patch on Phab, I'd be 
> happy to accept that!
> 
> Otherwise someone more familiar with x86 will have to take a look at the 
> below changes.
This is a simple alias. Spliting it doesn't solve the initial problem. I think 
it's better to commit it together with the backend change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112143

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

[PATCH] D109751: [Clang] Support conversion between PPC double-double and IEEE float128

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Please provide a description for this patch which includes justification for 
why we want to allow conversion between the two types.
I am of the impression that allowing the two types to coexist in completely 
disjoint code should be fine, but I really don't see a compelling reason to 
allow conversions between the two types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109751

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};

aaron.ballman wrote:
> carlosgalvezp wrote:
> > whisperity wrote:
> > > carlosgalvezp wrote:
> > > > salman-javed-nz wrote:
> > > > > What's the prevalent style for class member initialization? `=` or 
> > > > > `{}`?
> > > > > cppcoreguidelines-prefer-member-initializer defaults to `{}` but I 
> > > > > have seen both types in the code.
> > > > I tried to find this info in the LLVM coding guidelines but didn't find 
> > > > anything, so I assume it's maybe up to developers' discretion.
> > > > 
> > > > I prefer using braced initialization, since it prevents implicit 
> > > > conversions:
> > > > https://godbolt.org/z/4sP4rGsrY
> > > > 
> > > > More strict guidelines like Autosar enforce this. Also 
> > > > CppCoreGuidelines prefer that style as you point out.
> > > I think this is such a new and within LLVM relatively unused feature 
> > > (remember we are still pegged to C++14...) that we do not have a 
> > > consensus on style, and perhaps warrants discussing it on the mailing 
> > > list.
> > Just to clarify - this is C++11/14 syntax, nothing really fancy. I'm happy 
> > to change to " = 0" if people want, I don't have a strong opinion here.
> I've seen both styles but I believe I've seen `=` used more often for scalar 
> initialization and `{}` used more often for ctor initialization. e.g.,
> ```
> int a = 0; // More often
> int a{0}; // Less often
> 
> ClassFoo c = {1, 2, "three"}; // Less often
> ClassFoo c{1, 2, "three"}; // More often
> ```
Thanks for the info! IMO it's nice to have 1 way of initializing things 
consistently. Brace initialization is called "uniform initialization" for that 
reason - with the aim of uniforming the way of initializing data.

As said I have no strong opinion here, so I'll do what you guys tell me :) 



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:61-64
-  unsigned errorsIgnored() const {
+  int errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }

aaron.ballman wrote:
> The type changes overflow here from well-defined behavior to undefined 
> behavior. I don't think that's a serious concern (we have error limits 
> already), but in general, I don't think we should be changing types like this 
> without stronger rationale. This particular bit feels like churn without 
> benefit -- what do we gain by introducing the possibility for UB here? (I'm 
> not strongly opposed, but I get worried when well-defined code turns into 
> potentially undefined code in the name of an NFC cleanup.)
Do we have coding guidelines for when to use signed vs unsigned? I have quite 
ugly past experiences using unsigned types for arithmetic and as "counters" so 
these kinds of things really catch my eye quickly. This goes in line with e.g. 
the [[ https://google.github.io/styleguide/cppguide.html#Integer_Types | Google 
style guide ]].

Yes, you are correct that signed int overflow is UB, but would that then be an 
argument for banning signed ints in the codebase at all? IMO what's more 
important is to know the domain in which this is being applied. Do we expect 
people to have more than 2^31 linter warnings? If we do, would 2^32 really 
solve the problem, or just delay it? Maybe the proper solution is to go to 
64-bit ints if that's an expected use case.

Anyway if this is a concern I'm happy to revert and take the discussion outside 
the patch. We used to be over-defensive in this topic as well (UB is not to be 
taken lightly) but realized that perhaps the problem lies somewhere else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113847

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};

carlosgalvezp wrote:
> aaron.ballman wrote:
> > carlosgalvezp wrote:
> > > whisperity wrote:
> > > > carlosgalvezp wrote:
> > > > > salman-javed-nz wrote:
> > > > > > What's the prevalent style for class member initialization? `=` or 
> > > > > > `{}`?
> > > > > > cppcoreguidelines-prefer-member-initializer defaults to `{}` but I 
> > > > > > have seen both types in the code.
> > > > > I tried to find this info in the LLVM coding guidelines but didn't 
> > > > > find anything, so I assume it's maybe up to developers' discretion.
> > > > > 
> > > > > I prefer using braced initialization, since it prevents implicit 
> > > > > conversions:
> > > > > https://godbolt.org/z/4sP4rGsrY
> > > > > 
> > > > > More strict guidelines like Autosar enforce this. Also 
> > > > > CppCoreGuidelines prefer that style as you point out.
> > > > I think this is such a new and within LLVM relatively unused feature 
> > > > (remember we are still pegged to C++14...) that we do not have a 
> > > > consensus on style, and perhaps warrants discussing it on the mailing 
> > > > list.
> > > Just to clarify - this is C++11/14 syntax, nothing really fancy. I'm 
> > > happy to change to " = 0" if people want, I don't have a strong opinion 
> > > here.
> > I've seen both styles but I believe I've seen `=` used more often for 
> > scalar initialization and `{}` used more often for ctor initialization. 
> > e.g.,
> > ```
> > int a = 0; // More often
> > int a{0}; // Less often
> > 
> > ClassFoo c = {1, 2, "three"}; // Less often
> > ClassFoo c{1, 2, "three"}; // More often
> > ```
> Thanks for the info! IMO it's nice to have 1 way of initializing things 
> consistently. Brace initialization is called "uniform initialization" for 
> that reason - with the aim of uniforming the way of initializing data.
> 
> As said I have no strong opinion here, so I'll do what you guys tell me :) 
I usually refer to it as "unicorn initialization" because there's not nearly as 
uniform as people might expect. :-D I don't have strong preferences here though.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:61-64
-  unsigned errorsIgnored() const {
+  int errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }

carlosgalvezp wrote:
> aaron.ballman wrote:
> > The type changes overflow here from well-defined behavior to undefined 
> > behavior. I don't think that's a serious concern (we have error limits 
> > already), but in general, I don't think we should be changing types like 
> > this without stronger rationale. This particular bit feels like churn 
> > without benefit -- what do we gain by introducing the possibility for UB 
> > here? (I'm not strongly opposed, but I get worried when well-defined code 
> > turns into potentially undefined code in the name of an NFC cleanup.)
> Do we have coding guidelines for when to use signed vs unsigned? I have quite 
> ugly past experiences using unsigned types for arithmetic and as "counters" 
> so these kinds of things really catch my eye quickly. This goes in line with 
> e.g. the [[ https://google.github.io/styleguide/cppguide.html#Integer_Types | 
> Google style guide ]].
> 
> Yes, you are correct that signed int overflow is UB, but would that then be 
> an argument for banning signed ints in the codebase at all? IMO what's more 
> important is to know the domain in which this is being applied. Do we expect 
> people to have more than 2^31 linter warnings? If we do, would 2^32 really 
> solve the problem, or just delay it? Maybe the proper solution is to go to 
> 64-bit ints if that's an expected use case.
> 
> Anyway if this is a concern I'm happy to revert and take the discussion 
> outside the patch. We used to be over-defensive in this topic as well (UB is 
> not to be taken lightly) but realized that perhaps the problem lies somewhere 
> else.
> Do we have coding guidelines for when to use signed vs unsigned?

We don't, and I don't think we could even if we wanted to. There are times when 
unsigned is correct and there are times when signed is correct, and no blanket 
rule covers all the cases.

> Yes, you are correct that signed int overflow is UB, but would that then be 
> an argument for banning signed ints in the codebase at all? 

No. My point about the change to UB wasn't "I think this is dangerous" -- as I 
mentioned, we have limits on the number of errors that can be emitted, so I 
don't believe we can hit the UB in practice. It's more that changing 

[PATCH] D107994: Making the code compliant to the documentation about Floating Point support default values for C/C++. FPP-MODEL=PRECISE enables FFP-CONTRACT (FMA is enabled).

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

In D107994#3130494 , @wristow wrote:

> The Release Note change here says:
>
>   Floating Point Support in Clang
>   ---
>   - The -ffp-model=precise now implies -ffp-contract=on rather than
> -ffp-contract=fast, and the documentation of these features has been
> clarified. Previously, the documentation claimed that -ffp-model=precise 
> was
> the default, but this was incorrect because the precise model implied
> -ffp-contract=fast, whereas the default behavior is -ffp-contract=on.
> -ffp-model=precise is now exactly the default mode of the compiler.
>
> Unless I'm missing something, there is a related change here that I think 
> should be more overtly noted (given the discussions in this review, I 
> //think// this additional change is understood/expected, but I'm surprised 
> it's not pointed out explicitly -- so maybe I'm misunderstanding).
>
> Specifically, this commit explicitly sets `-ffp-contract=on` in the default 
> mode (which is what the documentation said, and continues to say).  But 
> previously, there wasn't //any// explicit setting of `-ffp-contract` by 
> default (and I think that lack of an explicit setting, was equivalent to 
> `-ffp-contract=off`).
>
> So with this commit, we now enable FMA by default (even at `-O0`). Noting the 
> semantic change that FMA is now being enabled by default seems sensible.
>
> Succinctly, in terms of the Release Note, the documentation claims that 
> `-ffp-contract=on` is the default, but in fact the behavior //was// as though 
> `-ffp-contract=off` was the default.  The default now really is 
> `-ffp-contract=on`.
>
> __
>
> Also, I see a relatively minor point about `-ffp-contract` in the Users 
> Manual.  It says that setting `-ffast-math` implies `-ffp-contract=fast`, and 
> it says that setting `-ffp-model=fast` "Behaves identically to specifying 
> both `-ffast-math` and `ffp-contract=fast`".  But that's redundant, since 
> `-ffast-math` already implies  `-ffp-contract=fast`.  That is, `-ffast-math` 
> and `-ffp-model=fast` are equivalent.

@wristow Are you suggesting a change of wording in the ReleaseNotes?


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

https://reviews.llvm.org/D107994

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 387231.
carlosgalvezp edited the summary of this revision.
carlosgalvezp added a comment.

- Use = initialization.
- Revert to unsigned.


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

https://reviews.llvm.org/D113847

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -45,18 +45,13 @@
   std::vector EnabledDiagnosticAliases;
 };
 
-/// Contains displayed and ignored diagnostic counters for a ClangTidy
-/// run.
+/// Contains displayed and ignored diagnostic counters for a ClangTidy run.
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
-
-  unsigned ErrorsDisplayed;
-  unsigned ErrorsIgnoredCheckFilter;
-  unsigned ErrorsIgnoredNOLINT;
-  unsigned ErrorsIgnoredNonUserCode;
-  unsigned ErrorsIgnoredLineFilter;
+  unsigned ErrorsDisplayed = 0;
+  unsigned ErrorsIgnoredCheckFilter = 0;
+  unsigned ErrorsIgnoredNOLUNSIGNED = 0;
+  unsigned ErrorsIgnoredNonUserCode = 0;
+  unsigned ErrorsIgnoredLineFilter = 0;
 
   unsigned errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -45,18 +45,13 @@
   std::vector EnabledDiagnosticAliases;
 };
 
-/// Contains displayed and ignored diagnostic counters for a ClangTidy
-/// run.
+/// Contains displayed and ignored diagnostic counters for a ClangTidy run.
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
-
-  unsigned ErrorsDisplayed;
-  unsigned ErrorsIgnoredCheckFilter;
-  unsigned ErrorsIgnoredNOLINT;
-  unsigned ErrorsIgnoredNonUserCode;
-  unsigned ErrorsIgnoredLineFilter;
+  unsigned ErrorsDisplayed = 0;
+  unsigned ErrorsIgnoredCheckFilter = 0;
+  unsigned ErrorsIgnoredNOLUNSIGNED = 0;
+  unsigned ErrorsIgnoredNonUserCode = 0;
+  unsigned ErrorsIgnoredLineFilter = 0;
 
   unsigned errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};

aaron.ballman wrote:
> carlosgalvezp wrote:
> > aaron.ballman wrote:
> > > carlosgalvezp wrote:
> > > > whisperity wrote:
> > > > > carlosgalvezp wrote:
> > > > > > salman-javed-nz wrote:
> > > > > > > What's the prevalent style for class member initialization? `=` 
> > > > > > > or `{}`?
> > > > > > > cppcoreguidelines-prefer-member-initializer defaults to `{}` but 
> > > > > > > I have seen both types in the code.
> > > > > > I tried to find this info in the LLVM coding guidelines but didn't 
> > > > > > find anything, so I assume it's maybe up to developers' discretion.
> > > > > > 
> > > > > > I prefer using braced initialization, since it prevents implicit 
> > > > > > conversions:
> > > > > > https://godbolt.org/z/4sP4rGsrY
> > > > > > 
> > > > > > More strict guidelines like Autosar enforce this. Also 
> > > > > > CppCoreGuidelines prefer that style as you point out.
> > > > > I think this is such a new and within LLVM relatively unused feature 
> > > > > (remember we are still pegged to C++14...) that we do not have a 
> > > > > consensus on style, and perhaps warrants discussing it on the mailing 
> > > > > list.
> > > > Just to clarify - this is C++11/14 syntax, nothing really fancy. I'm 
> > > > happy to change to " = 0" if people want, I don't have a strong opinion 
> > > > here.
> > > I've seen both styles but I believe I've seen `=` used more often for 
> > > scalar initialization and `{}` used more often for ctor initialization. 
> > > e.g.,
> > > ```
> > > int a = 0; // More often
> > > int a{0}; // Less often
> > > 
> > > ClassFoo c = {1, 2, "three"}; // Less often
> > > ClassFoo c{1, 2, "three"}; // More often
> > > ```
> > Thanks for the info! IMO it's nice to have 1 way of initializing things 
> > consistently. Brace initialization is called "uniform initialization" for 
> > that reason - with the aim of uniforming the way of initializing data.
> > 
> > As said I have no strong opinion here, so I'll do what you guys tell me :) 
> I usually refer to it as "unicorn initialization" because there's not nearly 
> as uniform as people might expect. :-D I don't have strong preferences here 
> though.
:) 



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:61-64
-  unsigned errorsIgnored() const {
+  int errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }

aaron.ballman wrote:
> carlosgalvezp wrote:
> > aaron.ballman wrote:
> > > The type changes overflow here from well-defined behavior to undefined 
> > > behavior. I don't think that's a serious concern (we have error limits 
> > > already), but in general, I don't think we should be changing types like 
> > > this without stronger rationale. This particular bit feels like churn 
> > > without benefit -- what do we gain by introducing the possibility for UB 
> > > here? (I'm not strongly opposed, but I get worried when well-defined code 
> > > turns into potentially undefined code in the name of an NFC cleanup.)
> > Do we have coding guidelines for when to use signed vs unsigned? I have 
> > quite ugly past experiences using unsigned types for arithmetic and as 
> > "counters" so these kinds of things really catch my eye quickly. This goes 
> > in line with e.g. the [[ 
> > https://google.github.io/styleguide/cppguide.html#Integer_Types | Google 
> > style guide ]].
> > 
> > Yes, you are correct that signed int overflow is UB, but would that then be 
> > an argument for banning signed ints in the codebase at all? IMO what's more 
> > important is to know the domain in which this is being applied. Do we 
> > expect people to have more than 2^31 linter warnings? If we do, would 2^32 
> > really solve the problem, or just delay it? Maybe the proper solution is to 
> > go to 64-bit ints if that's an expected use case.
> > 
> > Anyway if this is a concern I'm happy to revert and take the discussion 
> > outside the patch. We used to be over-defensive in this topic as well (UB 
> > is not to be taken lightly) but realized that perhaps the problem lies 
> > somewhere else.
> > Do we have coding guidelines for when to use signed vs unsigned?
> 
> We don't, and I don't think we could even if we wanted to. There are times 
> when unsigned is correct and there are times when signed is correct, and no 
> blanket rule covers all the cases.
> 
> > Yes, you are correct that signed int overflow is UB, but would that then be 
> > an argument for banning signed ints in the codebase at all? 
> 
> No. My point a

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/GlobList.h:50-52
+/// A \p GlobList that caches search results, so that search is performed only
+/// once for the same query.
+class CachedGlobList {

whisperity wrote:
> If `CachedGlobList` //is-a// `GlobList`, wouldn't it be better to express the 
> inheritance relationship?
Yes, that makes total sense to me. I wasn't sure if it was OK to do the move + 
change the code itself in the same patch. Will fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113422

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

[PATCH] D108301: [MSP430][Clang] Update hard-coded MCU data

[Jozef Lawrynowicz via Phabricator via cfe-commits]

jozefl added a comment.

Ping.

Thanks,
Jozef


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108301

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

[PATCH] D109174: [MSP430][Clang] Infer CPU type from -mcpu= or -mmcu=

[Jozef Lawrynowicz via Phabricator via cfe-commits]

jozefl added a comment.

Ping.

Thanks,
Jozef


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109174

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

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This broke building ANGLE as part of Qt 5.15 for a mingw target, with the 
following error:

  
../../../3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp:532:38:
 error: explicit instantiation of 'allocate' does not refer to a function 
template, variable template, member function, member class, or static data 
member
  ANGLE_RESOURCE_TYPE_OP(Instantitate, ANGLE_INSTANTIATE_OP)
   ^
  
../../../3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.h:301:15:
 note: candidate template ignored: could not match 'GetInitDataFromD3D11' 
(aka 'typename 
InitDataType::Value>::Value') against 
'const D3D11_SUBRESOURCE_DATA' 
  gl::Error allocate(Renderer11 *renderer,
^
  
../../../3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp:358:30:
 note: candidate template ignored: could not match 'GetInitDataFromD3D11' 
(aka 'typename 
InitDataType::Value>::Value') against 
'const D3D11_SUBRESOURCE_DATA'
  gl::Error ResourceManager11::allocate(Renderer11 *renderer,
   ^

Do you happen to know what's going on here? The files mentioned are 
https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp?h=5.15.1
 and 
https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.h?h=5.15.1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

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

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D110216#3130193 , @mizvekov wrote:

> In D110216#3130184 , @mstorsjo 
> wrote:
>
>> Do you happen to know what's going on here? The files mentioned are 
>> https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp?h=5.15.1
>>  and 
>> https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.h?h=5.15.1.
>
> Thanks for the report!
>
> Not immediately obvious to me, would need some time to isolate / create a 
> minimal test case.
> Would you be in a position to extract a preprocessed source file + command 
> line which repros this error?

Sure: https://martin.st/temp/angle-preproc.cpp, built with `clang -target 
x86_64-w64-mingw32 -std=c++17 -w -c angle-preproc.cpp`.

> Also, feel free to revert if it helps you.

No big hurry for me wrt that yet, at least until we know which part is at fault 
here.

(I haven't tested with the very latest Qt and/or ANGLE - the full build setup 
for this is rather complex.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

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

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3130184 , @mstorsjo wrote:

> Do you happen to know what's going on here? The files mentioned are 
> https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp?h=5.15.1
>  and 
> https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.h?h=5.15.1.

Thanks for the report!

Not immediately obvious to me, would need some time to isolate / create a 
minimal test case.
Would you be in a position to extract a preprocessed source file + command line 
which repros this error?

Also, feel free to revert if it helps you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

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

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Robert Richmond via Phabricator via cfe-commits]

RobRich999 added a comment.

Hit libANGLE in Chromium today, too.

https://bugs.chromium.org/p/chromium/issues/detail?id=1270154


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

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

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov reopened this revision.
mizvekov added a comment.

Reverting for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

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

[PATCH] D112430: [ARM][libunwind] add PACBTI-M support for libunwind

[Ties Stuij via Phabricator via cfe-commits]

stuij updated this revision to Diff 387237.
stuij added a comment.

addressed review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112430

Files:
  clang/lib/Headers/unwind.h
  libunwind/include/libunwind.h
  libunwind/include/unwind_arm_ehabi.h
  libunwind/src/DwarfInstructions.hpp
  libunwind/src/Registers.hpp
  libunwind/src/Unwind-EHABI.cpp
  libunwind/src/UnwindCursor.hpp
  libunwind/src/UnwindRegistersRestore.S
  libunwind/src/assembly.h

Index: libunwind/src/assembly.h
===
--- libunwind/src/assembly.h
+++ libunwind/src/assembly.h
@@ -81,7 +81,7 @@
 #define PPC64_OPD2
 #endif
 
-#if defined(__ARM_FEATURE_BTI_DEFAULT)
+#if defined(__aarch64__) && defined(__ARM_FEATURE_BTI_DEFAULT)
   .pushsection ".note.gnu.property", "a" SEPARATOR \
   .balign 8 SEPARATOR  \
   .long 4 SEPARATOR\
@@ -99,6 +99,17 @@
 #define AARCH64_BTI
 #endif
 
+#if !defined(__aarch64__)
+#ifdef __ARM_FEATURE_PAC_DEFAULT
+  .eabi_attribute Tag_PAC_extension, 2
+  .eabi_attribute Tag_PACRET_use, 1
+#endif
+#ifdef __ARM_FEATURE_BTI_DEFAULT
+  .eabi_attribute Tag_BTI_extension, 1
+  .eabi_attribute Tag_BTI_use, 1
+#endif
+#endif
+
 #define GLUE2(a, b) a ## b
 #define GLUE(a, b) GLUE2(a, b)
 #define SYMBOL_NAME(name) GLUE(__USER_LABEL_PREFIX__, name)
Index: libunwind/src/UnwindRegistersRestore.S
===
--- libunwind/src/UnwindRegistersRestore.S
+++ libunwind/src/UnwindRegistersRestore.S
@@ -660,7 +660,13 @@
   ldr sp, [lr, #52]
   ldr lr, [lr, #60]  @ restore pc into lr
 #endif
+#if defined(__ARM_FEATURE_BTI_DEFAULT) && !defined(__ARM_ARCH_ISA_ARM)
+  // 'bx' is not BTI setting when used with lr, therefore r12 is used instead
+  mov r12, lr
+  JMP(r12)
+#else
   JMP(lr)
+#endif
 
 @
 @ static void libunwind::Registers_arm::restoreVFPWithFLDMD(unw_fpreg_t* values)
Index: libunwind/src/UnwindCursor.hpp
===
--- libunwind/src/UnwindCursor.hpp
+++ libunwind/src/UnwindCursor.hpp
@@ -655,7 +655,9 @@
 #if defined(_LIBUNWIND_TARGET_X86_64)
   if (regNum >= UNW_X86_64_RAX && regNum <= UNW_X86_64_R15) return true;
 #elif defined(_LIBUNWIND_TARGET_ARM)
-  if (regNum >= UNW_ARM_R0 && regNum <= UNW_ARM_R15) return true;
+  if ((regNum >= UNW_ARM_R0 && regNum <= UNW_ARM_R15) ||
+  regNum == UNW_ARM_RA_AUTH_CODE)
+return true;
 #elif defined(_LIBUNWIND_TARGET_AARCH64)
   if (regNum >= UNW_AARCH64_X0 && regNum <= UNW_ARM64_X30) return true;
 #endif
Index: libunwind/src/Unwind-EHABI.cpp
===
--- libunwind/src/Unwind-EHABI.cpp
+++ libunwind/src/Unwind-EHABI.cpp
@@ -256,6 +256,7 @@
   size_t offset, size_t len) {
   bool wrotePC = false;
   bool finish = false;
+  bool hasReturnAddrAuthCode = false;
   while (offset < len && !finish) {
 uint8_t byte = getByte(data, offset++);
 if ((byte & 0x80) == 0) {
@@ -308,7 +309,7 @@
   if (offset >= len)
 return _URC_FAILURE;
   uint8_t registers = getByte(data, offset++);
-  if (registers & 0xf0 || !registers)
+  if (registers & 0xf0)
 return _URC_FAILURE;
   _Unwind_VRS_Pop(context, _UVRSC_CORE, registers, _UVRSD_UINT32);
   break;
@@ -342,6 +343,10 @@
   break;
 }
 case 0xb4:
+  hasReturnAddrAuthCode = true;
+  _Unwind_VRS_Pop(context, _UVRSC_PSEUDO,
+  0 /* Return Address Auth Code */, _UVRSD_UINT32);
+  break;
 case 0xb5:
 case 0xb6:
 case 0xb7:
@@ -417,6 +422,16 @@
   if (!wrotePC) {
 uint32_t lr;
 _Unwind_VRS_Get(context, _UVRSC_CORE, UNW_ARM_LR, _UVRSD_UINT32, &lr);
+#ifdef __ARM_FEATURE_PAUTH
+if (hasReturnAddrAuthCode) {
+  uint32_t sp;
+  uint32_t pac;
+  _Unwind_VRS_Get(context, _UVRSC_CORE, UNW_ARM_SP, _UVRSD_UINT32, &sp);
+  _Unwind_VRS_Get(context, _UVRSC_PSEUDO, UNW_ARM_RA_AUTH_CODE, _UVRSD_UINT32,
+  &pac);
+  __asm__ __volatile__("autg %0, %1, %2" : : "r"(pac), "r"(lr), "r"(sp) :);
+}
+#endif
 _Unwind_VRS_Set(context, _UVRSC_CORE, UNW_ARM_IP, _UVRSD_UINT32, &lr);
   }
   return _URC_CONTINUE_UNWIND;
@@ -927,6 +942,15 @@
 case _UVRSC_WMMXD:
   break;
 #endif
+case _UVRSC_PSEUDO:
+  // There's only one pseudo-register, PAC, with regno == 0.
+  if (representation != _UVRSD_UINT32 || regno != 0)
+return _UVRSR_FAILED;
+  return __unw_set_reg(cursor, (unw_regnum_t)(UNW_ARM_RA_AUTH_CODE),
+   *(unw_word_t *)valuep) == UNW_ESUCCESS
+ 

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/GlobList.h:50-52
+/// A \p GlobList that caches search results, so that search is performed only
+/// once for the same query.
+class CachedGlobList {

carlosgalvezp wrote:
> whisperity wrote:
> > If `CachedGlobList` //is-a// `GlobList`, wouldn't it be better to express 
> > the inheritance relationship?
> Yes, that makes total sense to me. I wasn't sure if it was OK to do the move 
> + change the code itself in the same patch. Will fix!
On the other hand, do we intend to use the class polymorphically? I wonder if 
private inheritance would be a better choice. Otherwise I'll need to start 
making the functions virtual and so on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113422

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

[PATCH] D113889: [NFC] disabling clang-tidy check readability-identifier-naming in Protocol.h

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman.
kuhnel added subscribers: sammccall, hokein, adamcz, kbobyrev.
kuhnel published this revision for review.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The file follows the LSP syntax, so we're intentially deviating
from the LLVM coding standard.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113889

Files:
  clang-tools-extra/clangd/Protocol.h


Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -36,6 +36,11 @@
 #include 
 #include 
 
+/* This file is using the LSP syntax for identifier names which is different
+ from the LLVM coding standard. To avoid the clang-tidy warnings, we're
+ disabling one check here.*/
+// NOLINTBEGIN(readability-identifier-naming)
+
 namespace clang {
 namespace clangd {
 
@@ -1794,4 +1799,6 @@
 };
 } // namespace llvm
 
+// NOLINTEND(readability-identifier-naming)
+
 #endif


Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -36,6 +36,11 @@
 #include 
 #include 
 
+/* This file is using the LSP syntax for identifier names which is different
+ from the LLVM coding standard. To avoid the clang-tidy warnings, we're
+ disabling one check here.*/
+// NOLINTBEGIN(readability-identifier-naming)
+
 namespace clang {
 namespace clangd {
 
@@ -1794,4 +1799,6 @@
 };
 } // namespace llvm
 
+// NOLINTEND(readability-identifier-naming)
+
 #endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113889: [NFC] disabling clang-tidy check readability-identifier-naming in Protocol.h

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/Protocol.h:39
 
+/* This file is using the LSP syntax for identifier names which is different
+ from the LLVM coding standard. To avoid the clang-tidy warnings, we're

Nit: we're only using C-style comments for single-line parameter name 
"injection".

Context:

https://llvm.org/docs/CodingStandards.html#comment-formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113889

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

[clang-tools-extra] d896c9f - Fix an unused variable warning

[Kirstóf Umann via cfe-commits]

Author: Kirstóf Umann
Date: 2021-11-15T15:45:43+01:00
New Revision: d896c9f40a22690d80cdf88152adbe591fd83d90

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

LOG: Fix an unused variable warning

Added: 


Modified: 
clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp 
b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
index 35fe51f11cd8..d29e631641dc 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
@@ -184,7 +184,7 @@ const Expr *digThroughConstructorsConversions(const Expr 
*E) {
   // If this is a conversion (as iterators commonly convert into their const
   // iterator counterparts), dig through that as well.
   if (const auto *ME = dyn_cast(E))
-if (const auto *D = dyn_cast(ME->getMethodDecl()))
+if (isa(ME->getMethodDecl()))
   return 
digThroughConstructorsConversions(ME->getImplicitObjectArgument());
   return E;
 }



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

[PATCH] D113891: [NFC][clangd] cleaning up unused "using"

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman.
kuhnel added subscribers: hokein, sammccall, adamcz, kbobyrev.
kuhnel published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Cleaning up unused "using" declarations.
This patch was generated from automatically applyning clang-tidy fixes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113891

Files:
  clang-tools-extra/clangd/index/YAMLSerialization.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/IndexTests.cpp
  clang-tools-extra/clangd/unittests/RIFFTests.cpp


Index: clang-tools-extra/clangd/unittests/RIFFTests.cpp
===
--- clang-tools-extra/clangd/unittests/RIFFTests.cpp
+++ clang-tools-extra/clangd/unittests/RIFFTests.cpp
@@ -13,7 +13,6 @@
 namespace clang {
 namespace clangd {
 namespace {
-using ::testing::ElementsAre;
 
 TEST(RIFFTest, File) {
   riff::File File{riff::fourCC("test"),
Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -22,7 +22,6 @@
 
 using ::testing::_;
 using ::testing::AllOf;
-using ::testing::AnyOf;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::Pair;
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -33,7 +33,6 @@
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
-using ::testing::UnorderedElementsAreArray;
 
 class HeadersTest : public ::testing::Test {
 public:
Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -40,7 +40,6 @@
 using ::testing::HasSubstr;
 using ::testing::IsEmpty;
 using ::testing::Not;
-using ::testing::StartsWith;
 using ::testing::UnorderedElementsAre;
 
 TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -54,10 +54,8 @@
 namespace {
 
 using ::testing::AllOf;
-using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
-using ::testing::Gt;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::SizeIs;
Index: clang-tools-extra/clangd/index/YAMLSerialization.cpp
===
--- clang-tools-extra/clangd/index/YAMLSerialization.cpp
+++ clang-tools-extra/clangd/index/YAMLSerialization.cpp
@@ -73,7 +73,6 @@
 using clang::index::SymbolInfo;
 using clang::index::SymbolKind;
 using clang::index::SymbolLanguage;
-using clang::index::SymbolRole;
 using clang::tooling::CompileCommand;
 
 // Helper to (de)serialize the SymbolID. We serialize it as a hex string.


Index: clang-tools-extra/clangd/unittests/RIFFTests.cpp
===
--- clang-tools-extra/clangd/unittests/RIFFTests.cpp
+++ clang-tools-extra/clangd/unittests/RIFFTests.cpp
@@ -13,7 +13,6 @@
 namespace clang {
 namespace clangd {
 namespace {
-using ::testing::ElementsAre;
 
 TEST(RIFFTest, File) {
   riff::File File{riff::fourCC("test"),
Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -22,7 +22,6 @@
 
 using ::testing::_;
 using ::testing::AllOf;
-using ::testing::AnyOf;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::Pair;
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -33,7 +33,6 @@
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
-using ::testing::UnorderedElementsAreArray;
 
 class HeadersTest : public ::testing::Test {
 public:
Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
=

[clang] 485c193 - Regenerate acle_st1*.c tests

[Matt Devereau via cfe-commits]

Author: Matt Devereau
Date: 2021-11-15T15:07:52Z
New Revision: 485c193aa12addea13a0db12f4c6bc6252244319

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

LOG: Regenerate acle_st1*.c tests

Regenerate acle_st1*.c tests using update_cc_test_checks.py

Added: 


Modified: 
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1b.c
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1h.c
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1w.c

Removed: 




diff  --git a/clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1b.c 
b/clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1b.c
index 2ae45552b13b..957add02437c 100644
--- a/clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1b.c
+++ b/clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1b.c
@@ -12,294 +12,350 @@
 #define SVE_ACLE_FUNC(A1,A2,A3,A4) A1##A2##A3##A4
 #endif
 
+// CHECK-LABEL: @test_svst1b_s16(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[TMP0:%.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv8i1( [[PG:%.*]])
+// CHECK-NEXT:[[TMP1:%.*]] = trunc  [[DATA:%.*]] to 

+// CHECK-NEXT:call void @llvm.aarch64.sve.st1.nxv8i8( 
[[TMP1]],  [[TMP0]], i8* [[BASE:%.*]])
+// CHECK-NEXT:ret void
+//
 void test_svst1b_s16(svbool_t pg, int8_t *base, svint16_t data)
 {
-  // CHECK-LABEL: test_svst1b_s16
-  // CHECK-DAG: %[[PG:.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv8i1( %pg)
-  // CHECK-DAG: %[[DATA:.*]] = trunc  %data to 
-  // CHECK: call void @llvm.aarch64.sve.st1.nxv8i8( 
%[[DATA]],  %[[PG]], i8* %base)
-  // CHECK: ret void
   return SVE_ACLE_FUNC(svst1b,_s16,,)(pg, base, data);
 }
 
+// CHECK-LABEL: @test_svst1b_s32(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[TMP0:%.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv4i1( [[PG:%.*]])
+// CHECK-NEXT:[[TMP1:%.*]] = trunc  [[DATA:%.*]] to 

+// CHECK-NEXT:call void @llvm.aarch64.sve.st1.nxv4i8( 
[[TMP1]],  [[TMP0]], i8* [[BASE:%.*]])
+// CHECK-NEXT:ret void
+//
 void test_svst1b_s32(svbool_t pg, int8_t *base, svint32_t data)
 {
-  // CHECK-LABEL: test_svst1b_s32
-  // CHECK-DAG: %[[PG:.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv4i1( %pg)
-  // CHECK-DAG: %[[DATA:.*]] = trunc  %data to 
-  // CHECK: call void @llvm.aarch64.sve.st1.nxv4i8( 
%[[DATA]],  %[[PG]], i8* %base)
-  // CHECK: ret void
   return SVE_ACLE_FUNC(svst1b,_s32,,)(pg, base, data);
 }
 
+// CHECK-LABEL: @test_svst1b_s64(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[TMP0:%.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv2i1( [[PG:%.*]])
+// CHECK-NEXT:[[TMP1:%.*]] = trunc  [[DATA:%.*]] to 

+// CHECK-NEXT:call void @llvm.aarch64.sve.st1.nxv2i8( 
[[TMP1]],  [[TMP0]], i8* [[BASE:%.*]])
+// CHECK-NEXT:ret void
+//
 void test_svst1b_s64(svbool_t pg, int8_t *base, svint64_t data)
 {
-  // CHECK-LABEL: test_svst1b_s64
-  // CHECK-DAG: %[[PG:.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv2i1( %pg)
-  // CHECK-DAG: %[[DATA:.*]] = trunc  %data to 
-  // CHECK: call void @llvm.aarch64.sve.st1.nxv2i8( 
%[[DATA]],  %[[PG]], i8* %base)
-  // CHECK: ret void
   return SVE_ACLE_FUNC(svst1b,_s64,,)(pg, base, data);
 }
 
+// CHECK-LABEL: @test_svst1b_u16(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[TMP0:%.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv8i1( [[PG:%.*]])
+// CHECK-NEXT:[[TMP1:%.*]] = trunc  [[DATA:%.*]] to 

+// CHECK-NEXT:call void @llvm.aarch64.sve.st1.nxv8i8( 
[[TMP1]],  [[TMP0]], i8* [[BASE:%.*]])
+// CHECK-NEXT:ret void
+//
 void test_svst1b_u16(svbool_t pg, uint8_t *base, svuint16_t data)
 {
-  // CHECK-LABEL: test_svst1b_u16
-  // CHECK-DAG: %[[PG:.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv8i1( %pg)
-  // CHECK-DAG: %[[DATA:.*]] = trunc  %data to 
-  // CHECK: call void @llvm.aarch64.sve.st1.nxv8i8( 
%[[DATA]],  %[[PG]], i8* %base)
-  // CHECK: ret void
   return SVE_ACLE_FUNC(svst1b,_u16,,)(pg, base, data);
 }
 
+// CHECK-LABEL: @test_svst1b_u32(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[TMP0:%.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv4i1( [[PG:%.*]])
+// CHECK-NEXT:[[TMP1:%.*]] = trunc  [[DATA:%.*]] to 

+// CHECK-NEXT:call void @llvm.aarch64.sve.st1.nxv4i8( 
[[TMP1]],  [[TMP0]], i8* [[BASE:%.*]])
+// CHECK-NEXT:ret void
+//
 void test_svst1b_u32(svbool_t pg, uint8_t *base, svuint32_t data)
 {
-  // CHECK-LABEL: test_svst1b_u32
-  // CHECK-DAG: %[[PG:.*]] = call  
@llvm.aarch64.sve.convert.from.svbool.nxv4i1( %pg)
-  // CHECK-DAG: %[[DATA:.*]] = trunc  %data to 
-  // CHECK: call void @llvm.aarch64.sve.st1.nxv4i8( 
%[[DATA]],  %[[PG]], i8* %base)
-  // CHECK: ret void
   return SVE_ACLE_FUNC(svst1b,_u32,,)(pg, base, data);
 }
 
+// CHECK-LABEL: @test_svst1b_u64(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[TMP0:%.*]] = call  
@llvm.aarch64.sve.convert

[PATCH] D113896: [NFC][clangd] cleanup of header guard names

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
Herald added subscribers: carlosgalvezp, wenlei, usaxena95, kadircet, arphaman.
kuhnel added subscribers: sammccall, adamcz, kbobyrev.
kuhnel published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Renaming header guards to match the LLVM convention.
This patch was created by automatically applying the fixes from
clang-tidy.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113896

Files:
  clang-tools-extra/clangd/CollectMacros.h
  clang-tools-extra/clangd/ExpectedTypes.h
  clang-tools-extra/clangd/FindTarget.h
  clang-tools-extra/clangd/HeaderSourceSwitch.h
  clang-tools-extra/clangd/HeuristicResolver.h
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/IncludeFixer.h
  clang-tools-extra/clangd/InlayHints.h
  clang-tools-extra/clangd/PathMapping.h
  clang-tools-extra/clangd/URI.h
  clang-tools-extra/clangd/index/BackgroundIndexLoader.h
  clang-tools-extra/clangd/index/BackgroundRebuild.h
  clang-tools-extra/clangd/index/CanonicalIncludes.h
  clang-tools-extra/clangd/index/IndexAction.h
  clang-tools-extra/clangd/index/ProjectAware.h
  clang-tools-extra/clangd/index/Serialization.h
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/index/SymbolLocation.h
  clang-tools-extra/clangd/index/SymbolOrigin.h
  clang-tools-extra/clangd/index/dex/Token.h
  clang-tools-extra/clangd/index/dex/Trigram.h
  clang-tools-extra/clangd/index/remote/Client.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/test/Inputs/BenchmarkHeader.h
  clang-tools-extra/clangd/test/Inputs/background-index/sub_dir/foo.h
  clang-tools-extra/clangd/test/Inputs/path-mappings/server/foo.h
  clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
  clang-tools-extra/clangd/test/remote-index/Inputs/Header.h
  clang-tools-extra/clangd/unittests/Annotations.h
  clang-tools-extra/clangd/unittests/LSPClient.h
  clang-tools-extra/clangd/unittests/Matchers.h
  clang-tools-extra/clangd/unittests/SyncAPI.h
  clang-tools-extra/clangd/unittests/TestFS.h
  clang-tools-extra/clangd/unittests/TestIndex.h
  clang-tools-extra/clangd/unittests/TestScheme.h
  clang-tools-extra/clangd/unittests/TestTU.h
  clang-tools-extra/clangd/unittests/TestWorkspace.h
  clang-tools-extra/clangd/unittests/decision_forest_model/CategoricalFeature.h
  clang-tools-extra/clangd/unittests/support/TestTracer.h
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h

Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -6,8 +6,8 @@
 //
 //===--===//
 
-#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TWEAKTESTING_H
-#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TWEAKTESTING_H
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_TWEAKS_TWEAKTESTING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_TWEAKS_TWEAKTESTING_H
 
 #include "TestTU.h"
 #include "index/Index.h"
Index: clang-tools-extra/clangd/unittests/support/TestTracer.h
===
--- clang-tools-extra/clangd/unittests/support/TestTracer.h
+++ clang-tools-extra/clangd/unittests/support/TestTracer.h
@@ -10,8 +10,8 @@
 //
 //===--===//
 
-#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SUPPORT_TESTTRACER_H
-#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SUPPORT_TESTTRACER_H
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_SUPPORT_TESTTRACER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_SUPPORT_TESTTRACER_H
 
 #include "support/Trace.h"
 #include "llvm/ADT/StringMap.h"
Index: clang-tools-extra/clangd/unittests/decision_forest_model/CategoricalFeature.h
===
--- clang-tools-extra/clangd/unittests/decision_forest_model/CategoricalFeature.h
+++ clang-tools-extra/clangd/unittests/decision_forest_model/CategoricalFeature.h
@@ -1,5 +1,10 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_DECISION_FOREST_MODEL_CATEGORICALFEATURE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_DECISION_FOREST_MODEL_CATEGORICALFEATURE_H
+
 namespace ns1 {
 namespace ns2 {
 enum TestEnum { A, B, C, D };
 } // namespace ns2
 } // namespace ns1
+
+#endif
Index: clang-tools-extra/clangd/unittests/TestWorkspace.h
===
--- clang-tools-extra/clangd/unittests/TestWorkspace.h
+++ clang-tools-extra/clangd/unittests/TestWorkspace.h
@@ -13,8 +13,8 @@
 //
 //===-===//
 
-#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTWORKSPACE_H
-#define LLVM_CLANG_TOOLS_EXTRA_UN

[PATCH] D113898: [NFC][clangd] cleaning up llvm-qualified-auto

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman, 
javed.absar.
kuhnel added subscribers: sammccall, adamcz, kbobyrev.
kuhnel published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This is a cleanup of all llvm-qualified-auto findings.
This patch was created by automatically applying the fixes from
clang-tidy.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113898

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/ExpectedTypes.cpp
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/HeaderSourceSwitch.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/dex/Iterator.cpp
  clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/refactor/tweaks/DumpAST.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp
@@ -192,7 +192,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformNestedNamespaces) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 namespace a {
   void bar();
   namespace b {
@@ -220,7 +220,7 @@
   b::c::aux();
   a::b::c::aux();
 })cpp";
-  auto Expected = R"cpp(
+  const auto *Expected = R"cpp(
 namespace a {
   void bar();
   namespace b {
@@ -252,7 +252,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformUsings) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 namespace a { namespace b { namespace c { void aux(); } } }
 
 void foo();
@@ -263,7 +263,7 @@
   using c::aux;
   namespace d = c;
 })cpp";
-  auto Expected = R"cpp(
+  const auto *Expected = R"cpp(
 namespace a { namespace b { namespace c { void aux(); } } }
 
 void foo(){
@@ -278,7 +278,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformDecls) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 void foo();
 void f^oo() {
   class Foo {
@@ -293,7 +293,7 @@
   enum class EnClass { Zero, One };
   EnClass y = EnClass::Zero;
 })cpp";
-  auto Expected = R"cpp(
+  const auto *Expected = R"cpp(
 void foo(){
   class Foo {
   public:
@@ -312,7 +312,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformTemplDecls) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -329,7 +329,7 @@
   bar>.bar();
   aux>();
 })cpp";
-  auto Expected = R"cpp(
+  const auto *Expected = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -350,7 +350,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformMembers) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 class Foo {
   void foo();
 };
@@ -358,7 +358,7 @@
 void Foo::f^oo() {
   return;
 })cpp";
-  auto Expected = R"cpp(
+  const auto *Expected = R"cpp(
 class Foo {
   void foo(){
   return;
@@ -395,7 +395,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformDependentTypes) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 namespace a {
   template  class Bar {};
 }
@@ -409,7 +409,7 @@
   Bar B;
   Bar> q;
 })cpp";
-  auto Expected = R"cpp(
+  const auto *Expected = R"cpp(
 namespace a {
   template  class Bar {};
 }
@@ -511,7 +511,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformTypeLocs) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -528,7 +528,7 @@
   Foo foo;
   a::Bar>::Baz> q;
 })cpp";
-  auto Expected = R"cpp(
+  const auto *Expected = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -549,7 +549,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformDeclRefs) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -575,7 +575,7 @@
   bar();
   a::test();
 })cpp";
-  auto Expected = R"cpp(
+  const auto *Expected = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -605,12 +605,12 @@
 }
 
 TEST_F(DefineInlineTest, StaticMembers) {
-  auto Test = R"cpp(
+  const auto *Test = R"cpp(
 namespace ns { class X { static void foo

[PATCH] D113891: [NFC][clangd] cleaning up unused "using"

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113891

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

[PATCH] D113776: [Clang][SVE] Properly enable/disable dependant SVE target features based upon +(no)sve.* options

[mgabka via Phabricator via cfe-commits]

mgabka added inline comments.



Comment at: clang/test/Driver/aarch64-implied-sve-features.c:28
+
+// RUN: %clang -target aarch64-linux-gnu -march=armv8-a+nosve2-bitperm %s -### 
|& FileCheck %s --check-prefix=NOSVE2-BITPERM
+// NOSVE2-BITPERM-NOT: "-target-feature" "+sve2-bitperm"

Hi,
@bsmith could you at at least one test for +nosve2-sha3, +nosve2-aes and 
+nosve2-sm4?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113776

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

Yes, the current approach of "-march=+feature" is terrible and does not 
work with developers who want flexibility of features. This being pitched as a 
feature imo is akin to promoting a design bug as a feature. 
Any additive or subtractive alternative is welcome.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113896: [NFC][clangd] cleanup of header guard names

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Mostly LG, just a few files are test inputs and one is empty




Comment at: 
clang-tools-extra/clangd/test/Inputs/background-index/sub_dir/foo.h:1
-#ifndef FOO_H
-#define FOO_H
+#ifndef 
LLVM_CLANG_TOOLS_EXTRA_CLANGD_TEST_INPUTS_BACKGROUND_INDEX_SUB_DIR_FOO_H
+#define 
LLVM_CLANG_TOOLS_EXTRA_CLANGD_TEST_INPUTS_BACKGROUND_INDEX_SUB_DIR_FOO_H

Please don't modify tests, they don't need to follow LLVM style and should be 
as simple as possible



Comment at: clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h:1
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TEST_INDEX_SERIALIZATION_INPUTS_SAMPLE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TEST_INDEX_SERIALIZATION_INPUTS_SAMPLE_H

In particular, adding header guards to test files without them isn't NFC



Comment at: clang-tools-extra/clangd/unittests/TestScheme.h:1
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_TESTSCHEME_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_TESTSCHEME_H

What's up with this change?
If this file is empty it should just be deleted instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113896

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

[PATCH] D112430: [ARM][libunwind] add PACBTI-M support for libunwind

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss accepted this revision.
danielkiss added a comment.

LGTM, Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112430

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

[clang] da168dd - [clang] Allow clang-check to customize analyzer output file or dir name

[Kirill Bobyrev via cfe-commits]

Author: Ella Ma
Date: 2021-11-15T16:49:41+01:00
New Revision: da168dd875bf0392e8e88834009d776bfbaae376

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

LOG: [clang] Allow clang-check to customize analyzer output file or dir name

Required by https://stackoverflow.com/questions/58073606

As the output argument is stripped out in the clang-check tool, it seems 
impossible for clang-check users to customize the output file name, even with 
-extra-args and -extra-arg-before.

This patch adds the -analyzer-output-path argument to allow users to adjust the 
output name. And if the argument is not set or the analyzer is not enabled, the 
original strip output adjuster will remove the output arguments.

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

Added: 
clang/test/Tooling/clang-check-set-analyzer-output-path.cpp

Modified: 
clang/tools/clang-check/ClangCheck.cpp

Removed: 




diff  --git a/clang/test/Tooling/clang-check-set-analyzer-output-path.cpp 
b/clang/test/Tooling/clang-check-set-analyzer-output-path.cpp
new file mode 100644
index ..ff5e86ae7892
--- /dev/null
+++ b/clang/test/Tooling/clang-check-set-analyzer-output-path.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: echo '[{"directory":".","command":"clang++ -c %t/test.cpp -o foo 
-ofoo","file":"%t/test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: echo '// CHECK: {{qwerty}}' > %t/cclog-check
+// RUN: clang-check -p "%t" "%t/test.cpp" -analyze 
-analyzer-output-path=%t/qwerty -extra-arg=-v -extra-arg=-Xclang 
-extra-arg=-verify 2>&1 | FileCheck %t/cclog-check
+// RUN: FileCheck %s --input-file=%t/qwerty
+
+// CHECK: DOCTYPE plist
+// CHECK: Division by zero
+int f() {
+  return 1 / 0; // expected-warning {{Division by zero}}
+}

diff  --git a/clang/tools/clang-check/ClangCheck.cpp 
b/clang/tools/clang-check/ClangCheck.cpp
index 11fdeb71fd9e..4d6ded029e2f 100644
--- a/clang/tools/clang-check/ClangCheck.cpp
+++ b/clang/tools/clang-check/ClangCheck.cpp
@@ -76,6 +76,10 @@ static cl::opt
 Analyze("analyze",
 cl::desc(Options.getOptionHelpText(options::OPT_analyze)),
 cl::cat(ClangCheckCategory));
+static cl::opt
+AnalyzerOutput("analyzer-output-path",
+   cl::desc(Options.getOptionHelpText(options::OPT_o)),
+   cl::cat(ClangCheckCategory));
 
 static cl::opt
 Fixit("fixit", cl::desc(Options.getOptionHelpText(options::OPT_fixit)),
@@ -206,7 +210,19 @@ int main(int argc, const char **argv) {
 
   // Clear adjusters because -fsyntax-only is inserted by the default chain.
   Tool.clearArgumentsAdjusters();
-  Tool.appendArgumentsAdjuster(getClangStripOutputAdjuster());
+
+  // Reset output path if is provided by user.
+  Tool.appendArgumentsAdjuster(
+  Analyze ? [&](const CommandLineArguments &Args, StringRef File) {
+  auto Ret = getClangStripOutputAdjuster()(Args, File);
+  if (!AnalyzerOutput.empty()) {
+Ret.emplace_back("-o");
+Ret.emplace_back(AnalyzerOutput);
+  }
+  return Ret;
+}
+  : getClangStripOutputAdjuster());
+
   Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
 
   // Running the analyzer requires --analyze. Other modes can work with the



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

[PATCH] D97265: [clang] Allow clang-check to customize analyzer output file or dir name

[Kirill Bobyrev 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 rGda168dd875bf: [clang] Allow clang-check to customize 
analyzer output file or dir name (authored by OikawaKirie, committed by 
kbobyrev).
Herald added a subscriber: manas.

Changed prior to commit:
  https://reviews.llvm.org/D97265?vs=328927&id=387266#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97265

Files:
  clang/test/Tooling/clang-check-set-analyzer-output-path.cpp
  clang/tools/clang-check/ClangCheck.cpp


Index: clang/tools/clang-check/ClangCheck.cpp
===
--- clang/tools/clang-check/ClangCheck.cpp
+++ clang/tools/clang-check/ClangCheck.cpp
@@ -76,6 +76,10 @@
 Analyze("analyze",
 cl::desc(Options.getOptionHelpText(options::OPT_analyze)),
 cl::cat(ClangCheckCategory));
+static cl::opt
+AnalyzerOutput("analyzer-output-path",
+   cl::desc(Options.getOptionHelpText(options::OPT_o)),
+   cl::cat(ClangCheckCategory));
 
 static cl::opt
 Fixit("fixit", cl::desc(Options.getOptionHelpText(options::OPT_fixit)),
@@ -206,7 +210,19 @@
 
   // Clear adjusters because -fsyntax-only is inserted by the default chain.
   Tool.clearArgumentsAdjusters();
-  Tool.appendArgumentsAdjuster(getClangStripOutputAdjuster());
+
+  // Reset output path if is provided by user.
+  Tool.appendArgumentsAdjuster(
+  Analyze ? [&](const CommandLineArguments &Args, StringRef File) {
+  auto Ret = getClangStripOutputAdjuster()(Args, File);
+  if (!AnalyzerOutput.empty()) {
+Ret.emplace_back("-o");
+Ret.emplace_back(AnalyzerOutput);
+  }
+  return Ret;
+}
+  : getClangStripOutputAdjuster());
+
   Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
 
   // Running the analyzer requires --analyze. Other modes can work with the
Index: clang/test/Tooling/clang-check-set-analyzer-output-path.cpp
===
--- /dev/null
+++ clang/test/Tooling/clang-check-set-analyzer-output-path.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: echo '[{"directory":".","command":"clang++ -c %t/test.cpp -o foo 
-ofoo","file":"%t/test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: echo '// CHECK: {{qwerty}}' > %t/cclog-check
+// RUN: clang-check -p "%t" "%t/test.cpp" -analyze 
-analyzer-output-path=%t/qwerty -extra-arg=-v -extra-arg=-Xclang 
-extra-arg=-verify 2>&1 | FileCheck %t/cclog-check
+// RUN: FileCheck %s --input-file=%t/qwerty
+
+// CHECK: DOCTYPE plist
+// CHECK: Division by zero
+int f() {
+  return 1 / 0; // expected-warning {{Division by zero}}
+}


Index: clang/tools/clang-check/ClangCheck.cpp
===
--- clang/tools/clang-check/ClangCheck.cpp
+++ clang/tools/clang-check/ClangCheck.cpp
@@ -76,6 +76,10 @@
 Analyze("analyze",
 cl::desc(Options.getOptionHelpText(options::OPT_analyze)),
 cl::cat(ClangCheckCategory));
+static cl::opt
+AnalyzerOutput("analyzer-output-path",
+   cl::desc(Options.getOptionHelpText(options::OPT_o)),
+   cl::cat(ClangCheckCategory));
 
 static cl::opt
 Fixit("fixit", cl::desc(Options.getOptionHelpText(options::OPT_fixit)),
@@ -206,7 +210,19 @@
 
   // Clear adjusters because -fsyntax-only is inserted by the default chain.
   Tool.clearArgumentsAdjusters();
-  Tool.appendArgumentsAdjuster(getClangStripOutputAdjuster());
+
+  // Reset output path if is provided by user.
+  Tool.appendArgumentsAdjuster(
+  Analyze ? [&](const CommandLineArguments &Args, StringRef File) {
+  auto Ret = getClangStripOutputAdjuster()(Args, File);
+  if (!AnalyzerOutput.empty()) {
+Ret.emplace_back("-o");
+Ret.emplace_back(AnalyzerOutput);
+  }
+  return Ret;
+}
+  : getClangStripOutputAdjuster());
+
   Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
 
   // Running the analyzer requires --analyze. Other modes can work with the
Index: clang/test/Tooling/clang-check-set-analyzer-output-path.cpp
===
--- /dev/null
+++ clang/test/Tooling/clang-check-set-analyzer-output-path.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: echo '[{"directory":".","command":"clang++ -c %t/test.cpp -o foo -ofoo","file":"%t/test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: echo '// CHECK: {{qwerty}}'

[PATCH] D97265: [clang] Allow clang-check to customize analyzer output file or dir name

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

Hey, I'm really sorry, I missed this, too but I just landed the patch for you, 
apologies for such a huge delay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97265

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

[clang] 0e73832 - [openmp][amdgpu] Add comment warning that libm may be broken

[Jon Chesterfield via cfe-commits]

Author: Jon Chesterfield
Date: 2021-11-15T15:56:01Z
New Revision: 0e738323a9c445e31b4e1b1dcb2beb19d6f103ef

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

LOG: [openmp][amdgpu] Add comment warning that libm may be broken

Using llvm-link to add rocm device-libs probably doesn't work

Reviewed By: arsenm

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

Added: 


Modified: 
clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp 
b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
index b138000f8cf2..863e2c597d53 100644
--- a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
@@ -106,6 +106,22 @@ const char *AMDGCN::OpenMPLinker::constructLLVMLinkCommand(
 }
 
 if (HasLibm) {
+  // This is not certain to work. The device libs added here, and passed to
+  // llvm-link, are missing attributes that they expect to be inserted when
+  // passed to mlink-builtin-bitcode. The amdgpu backend does not generate
+  // conservatively correct code when attributes are missing, so this may
+  // be the root cause of miscompilations. Passing via 
mlink-builtin-bitcode
+  // ultimately hits CodeGenModule::addDefaultFunctionDefinitionAttributes
+  // on each function, see D28538 for context.
+  // Potential workarounds:
+  //  - unconditionally link all of the device libs to every translation
+  //unit in clang via mlink-builtin-bitcode
+  //  - build a libm bitcode file as part of the DeviceRTL and explictly
+  //mlink-builtin-bitcode the rocm device libs components at build time
+  //  - drop this llvm-link fork in favour or some calls into LLVM, chosen
+  //to do basically the same work as llvm-link but with that call first
+  //  - write an opt pass that sets that on every function it sees and pipe
+  //the device-libs bitcode through that on the way to this llvm-link
   SmallVector BCLibs =
   AMDGPUOpenMPTC.getCommonDeviceLibNames(Args, SubArchName.str());
   llvm::for_each(BCLibs, [&](StringRef BCFile) {



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

[PATCH] D112639: [openmp][amdgpu] Add comment warning that libm may be broken

[Jon Chesterfield via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0e738323a9c4: [openmp][amdgpu] Add comment warning that libm 
may be broken (authored by JonChesterfield).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112639

Files:
  clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp


Index: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
===
--- clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
+++ clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
@@ -106,6 +106,22 @@
 }
 
 if (HasLibm) {
+  // This is not certain to work. The device libs added here, and passed to
+  // llvm-link, are missing attributes that they expect to be inserted when
+  // passed to mlink-builtin-bitcode. The amdgpu backend does not generate
+  // conservatively correct code when attributes are missing, so this may
+  // be the root cause of miscompilations. Passing via 
mlink-builtin-bitcode
+  // ultimately hits CodeGenModule::addDefaultFunctionDefinitionAttributes
+  // on each function, see D28538 for context.
+  // Potential workarounds:
+  //  - unconditionally link all of the device libs to every translation
+  //unit in clang via mlink-builtin-bitcode
+  //  - build a libm bitcode file as part of the DeviceRTL and explictly
+  //mlink-builtin-bitcode the rocm device libs components at build time
+  //  - drop this llvm-link fork in favour or some calls into LLVM, chosen
+  //to do basically the same work as llvm-link but with that call first
+  //  - write an opt pass that sets that on every function it sees and pipe
+  //the device-libs bitcode through that on the way to this llvm-link
   SmallVector BCLibs =
   AMDGPUOpenMPTC.getCommonDeviceLibNames(Args, SubArchName.str());
   llvm::for_each(BCLibs, [&](StringRef BCFile) {


Index: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
===
--- clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
+++ clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
@@ -106,6 +106,22 @@
 }
 
 if (HasLibm) {
+  // This is not certain to work. The device libs added here, and passed to
+  // llvm-link, are missing attributes that they expect to be inserted when
+  // passed to mlink-builtin-bitcode. The amdgpu backend does not generate
+  // conservatively correct code when attributes are missing, so this may
+  // be the root cause of miscompilations. Passing via mlink-builtin-bitcode
+  // ultimately hits CodeGenModule::addDefaultFunctionDefinitionAttributes
+  // on each function, see D28538 for context.
+  // Potential workarounds:
+  //  - unconditionally link all of the device libs to every translation
+  //unit in clang via mlink-builtin-bitcode
+  //  - build a libm bitcode file as part of the DeviceRTL and explictly
+  //mlink-builtin-bitcode the rocm device libs components at build time
+  //  - drop this llvm-link fork in favour or some calls into LLVM, chosen
+  //to do basically the same work as llvm-link but with that call first
+  //  - write an opt pass that sets that on every function it sees and pipe
+  //the device-libs bitcode through that on the way to this llvm-link
   SmallVector BCLibs =
   AMDGPUOpenMPTC.getCommonDeviceLibNames(Args, SubArchName.str());
   llvm::for_each(BCLibs, [&](StringRef BCFile) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113898: [NFC][clangd] cleaning up llvm-qualified-auto

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

There is a reasonable style rule here, but:

- the suggested fixes spell out `const` in ways we generally prefer not to. 
(And the rule doesn't require).
- Strings are a special case. `auto*` for string literals makes no sense to me, 
the check should be modified to either accept `auto` or replace with `const 
char*`




Comment at: clang-tools-extra/clangd/AST.cpp:441
   DeducedType = AT->getDeducedType();
-} else if (auto DT = dyn_cast(D->getReturnType())) {
+} else if (const auto *DT = dyn_cast(D->getReturnType())) {
   // auto in a trailing return type just points to a DecltypeType and

We usually use `auto*` rather than `const auto*` for locals like this.

For the same reason we don't `const` locals: it adds more noise than it helps.
(And if it's critical we spell out these things, we probably shouldn't be using 
auto at all)

When this check was introduced, there was a discussion about this and they 
agreed to have the check stop flagging `auto*`, but if it sees `auto` it will 
still suggest the const. So the automatic fixes generally aren't what we want, 
we need to drop const.



Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:64
 const auto FileID = SM.getFileID(Loc);
-const auto File = SM.getFileEntryForID(FileID);
+const auto *const File = SM.getFileEntryForID(FileID);
 auto URI = toURI(File);

This is a particularly confusing type.
The original code is atypical for clangd, I'd suggest changing to `auto* File`



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:209
 OS << "(| ";
-auto Separator = "";
+const auto *Separator = "";
 for (const auto &Child : Children) {

`const char*` or `auto` or `StringRef` for strings. "Some kind of pointer" is 
technically correct but unhelpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113898

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

[PATCH] D113902: [NFC][clangd] exclude test data from clang-tidy

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
kuhnel added subscribers: sammccall, adamcz, kbobyrev.
kuhnel published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov, aheejin.
Herald added a project: clang-tools-extra.

Adding .clang-tidy files to exclude test data from clang-tidy checks. Otherwise 
these create false positives.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113902

Files:
  clang-tools-extra/clangd/test/Inputs/.clang-tidy
  clang-tools-extra/clangd/test/index-serialization/Inputs/.clang-tidy
  clang-tools-extra/clangd/test/remote-index/Inputs/.clang-tidy


Index: clang-tools-extra/clangd/test/remote-index/Inputs/.clang-tidy
===
--- /dev/null
+++ clang-tools-extra/clangd/test/remote-index/Inputs/.clang-tidy
@@ -0,0 +1,3 @@
+# disable the check on the test input data
+InheritParentConfig: true
+Checks: "-misc-definitions-in-headers"
Index: clang-tools-extra/clangd/test/index-serialization/Inputs/.clang-tidy
===
--- /dev/null
+++ clang-tools-extra/clangd/test/index-serialization/Inputs/.clang-tidy
@@ -0,0 +1,3 @@
+# disable the check on the test input data
+InheritParentConfig: true
+Checks: "-clang-diagnostic-non-virtual-dtor,-readability-identifier-naming"
Index: clang-tools-extra/clangd/test/Inputs/.clang-tidy
===
--- /dev/null
+++ clang-tools-extra/clangd/test/Inputs/.clang-tidy
@@ -0,0 +1,3 @@
+# disable the check on the test input data
+InheritParentConfig: true
+Checks: 
"-misc-definitions-in-headers,-llvm-header-guard,-clang-diagnostic-unused-variable"


Index: clang-tools-extra/clangd/test/remote-index/Inputs/.clang-tidy
===
--- /dev/null
+++ clang-tools-extra/clangd/test/remote-index/Inputs/.clang-tidy
@@ -0,0 +1,3 @@
+# disable the check on the test input data
+InheritParentConfig: true
+Checks: "-misc-definitions-in-headers"
Index: clang-tools-extra/clangd/test/index-serialization/Inputs/.clang-tidy
===
--- /dev/null
+++ clang-tools-extra/clangd/test/index-serialization/Inputs/.clang-tidy
@@ -0,0 +1,3 @@
+# disable the check on the test input data
+InheritParentConfig: true
+Checks: "-clang-diagnostic-non-virtual-dtor,-readability-identifier-naming"
Index: clang-tools-extra/clangd/test/Inputs/.clang-tidy
===
--- /dev/null
+++ clang-tools-extra/clangd/test/Inputs/.clang-tidy
@@ -0,0 +1,3 @@
+# disable the check on the test input data
+InheritParentConfig: true
+Checks: "-misc-definitions-in-headers,-llvm-header-guard,-clang-diagnostic-unused-variable"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113899: [NFC][clangd] fix clang-tidy finding on isa_and_nonnull

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman.
kuhnel added subscribers: sammccall, kbobyrev, adamcz.
kadircet added inline comments.
kuhnel published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.



Comment at: clang-tools-extra/clangd/Selection.cpp:504
   bool TraverseDecl(Decl *X) {
-if (X && isa(X))
+if (isa_and_nonnull(X))
   return Base::TraverseDecl(X); // Already pushed by constructor.

while here `llvm::isa_and_nonnull` we try to qualify symbols from llvm 
namespace.


This is a cleanup of the only llvm-prefer-isa-or-dyn-cast-in-conditionals 
finding in the clangd code base. This patch was created by automatically 
applying the fixes from clang-tidy.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113899

Files:
  clang-tools-extra/clangd/Selection.cpp


Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -501,7 +501,7 @@
   //  - those without source range information, we don't record those
   //  - those that can't be stored in DynTypedNode.
   bool TraverseDecl(Decl *X) {
-if (X && isa(X))
+if (isa_and_nonnull(X))
   return Base::TraverseDecl(X); // Already pushed by constructor.
 // Base::TraverseDecl will suppress children, but not this node itself.
 if (X && X->isImplicit())


Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -501,7 +501,7 @@
   //  - those without source range information, we don't record those
   //  - those that can't be stored in DynTypedNode.
   bool TraverseDecl(Decl *X) {
-if (X && isa(X))
+if (isa_and_nonnull(X))
   return Base::TraverseDecl(X); // Already pushed by constructor.
 // Base::TraverseDecl will suppress children, but not this node itself.
 if (X && X->isImplicit())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113895: [NFC][clangd] fix llvm-namespace-comment finding

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman.
kuhnel published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Fixing the clang-tidy finding.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113895

Files:
  clang-tools-extra/clangd/xpc/XPCTransport.cpp


Index: clang-tools-extra/clangd/xpc/XPCTransport.cpp
===
--- clang-tools-extra/clangd/xpc/XPCTransport.cpp
+++ clang-tools-extra/clangd/xpc/XPCTransport.cpp
@@ -47,7 +47,7 @@
 // C "closure" for XPCTransport::loop() method
 namespace xpcClosure {
 void connection_handler(xpc_connection_t clientConnection);
-}
+} // namespace xpcClosure
 
 class XPCTransport : public Transport {
 public:


Index: clang-tools-extra/clangd/xpc/XPCTransport.cpp
===
--- clang-tools-extra/clangd/xpc/XPCTransport.cpp
+++ clang-tools-extra/clangd/xpc/XPCTransport.cpp
@@ -47,7 +47,7 @@
 // C "closure" for XPCTransport::loop() method
 namespace xpcClosure {
 void connection_handler(xpc_connection_t clientConnection);
-}
+} // namespace xpcClosure
 
 class XPCTransport : public Transport {
 public:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113892: [NFC][clangd] cleanup llvm-else-after-return findings

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman, 
javed.absar.
kuhnel added subscribers: sammccall, hokein, adamcz, kbobyrev.
kuhnel published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Cleanup of clang-tidy findings: removing "else" after a return statement
to improve readability of the code.

This patch was created by applying the clang-tidy fixes automatically.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113892

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/FuzzyMatch.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/JSONTransport.cpp
  clang-tools-extra/clangd/PathMapping.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -223,8 +223,7 @@
   while (const auto *CS = llvm::dyn_cast(Last)) {
 if (CS->body_empty())
   return false;
-else
-  Last = CS->body_back();
+Last = CS->body_back();
   }
   return llvm::isa(Last);
 }
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -250,7 +250,8 @@
   for (; Node->Parent; Node = Node->Parent) {
 if (Node->ASTNode.get()) {
   continue;
-} else if (auto *T = Node->ASTNode.get()) {
+}
+if (auto *T = Node->ASTNode.get()) {
   if (T->getAs()) {
 break;
   } else if (Node->Parent->ASTNode.get() ||
Index: clang-tools-extra/clangd/index/Serialization.cpp
===
--- clang-tools-extra/clangd/index/Serialization.cpp
+++ clang-tools-extra/clangd/index/Serialization.cpp
@@ -687,7 +687,8 @@
 llvm::Expected readIndexFile(llvm::StringRef Data) {
   if (Data.startswith("RIFF")) {
 return readRIFF(Data);
-  } else if (auto YAMLContents = readYAML(Data)) {
+  }
+  if (auto YAMLContents = readYAML(Data)) {
 return std::move(*YAMLContents);
   } else {
 return error("Not a RIFF file and failed to parse as YAML: {0}",
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -401,8 +401,8 @@
 }
   }
   // Special case: void foo() ^override: jump to the overridden method.
-if (NodeKind.isSame(ASTNodeKind::getFromNodeKind()) ||
-NodeKind.isSame(ASTNodeKind::getFromNodeKind())) {
+  if (NodeKind.isSame(ASTNodeKind::getFromNodeKind()) ||
+  NodeKind.isSame(ASTNodeKind::getFromNodeKind())) {
 // We may be overridding multiple methods - offer them all.
 for (const NamedDecl *ND : CMD->overridden_methods())
   AddResultDecl(ND);
@@ -826,10 +826,9 @@
 log("Found definition heuristically using nearby identifier {0}",
 NearbyIdent->text(SM));
 return ASTResults;
-  } else {
-vlog("No definition found using nearby identifier {0} at {1}",
- Word->Text, Word->Location.printToString(SM));
   }
+  vlog("No definition found using nearby identifier {0} at {1}", Word->Text,
+   Word->Location.printToString(SM));
 }
 // No nearby word, or it didn't refer to anything either. Try the index.
 auto TextualResults =
@@ -1430,28 +1429,27 @@
 // Add decl/def of overridding methods.
 if (Index && Results.References.size() <= Limit &&
 !OverriddenBy.Subjects.empty())
-  Index->relations(
-  OverriddenBy, [&](const SymbolID &Subject, const Symbol &Object) {
-const auto LSPLocDecl =
-toLSPLocation(Object.CanonicalDeclaration, *MainFilePath);
-const auto LSPLocDef =
-toLSPLocation(Object.Definition, *MainFilePath);
-if (LSPLocDecl && LSPLocDecl != LSPLocDef) {
-  ReferencesResult::Reference Result;
-  Result.Loc = std::move(*LSPLocDecl);
-  Result.Attributes =
-  ReferencesResult::Declaration | ReferencesResult::Override;
-  Results.References.push_back(std::move(Result));
-}
-if (LSPLocDef) {
-  ReferencesResult::Reference Result;
-  Result.Loc = std::move(*LSPLo

[PATCH] D113902: [NFC][clangd] exclude test data from clang-tidy

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hmm, clang-tidy should only be running on entries in compile_commands.json. are 
these files listed somehow?

(I don't like the idea that every test inputs dir may need to contain a 
scattering of files to disable tools. At most we should be able to do this 
under test/)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113902

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

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

FYI this is a minimal repro taken from Martin's preprocessed source (Thanks!):

  template  struct a { using b = const float; };
  template  using d = typename a::b;  
  
  template  void e(d *, c) {}
  template void e(typename a::b *, int);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

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

[PATCH] D113892: [NFC][clangd] cleanup llvm-else-after-return findings

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Mostly looks good. There's a bunch of ifs that are now trivial (condition and 
body each fit on one line, no else) and we'd usually drop braces there, but 
it's probably not worth the hassle.

I have to say that this rule slightly improves "early exit after error 
condition" type code, but makes it less clear that "there are 3 cases" type 
code is exhaustive.
So personally I'm not sure the check is a win if we want to follow it 
everywhere, I'd consider fixing the cases where it improves things and then 
turning it off. @kadircet am I being too picky here?




Comment at: clang-tools-extra/clangd/JSONTransport.cpp:260
   continue;
 } else {
   // An empty line indicates the end of headers.

This seems inconsistent: we're elseing after continue.

If we've using this style, i'd invert the if here so the "continue" case just 
falls off the end of the loop body. (But keep the comment)



Comment at: clang-tools-extra/clangd/JSONTransport.cpp:283
+Read = retryAfterSignalUnlessShutdown(
+0, [&] { return std::fread(&JSON[Pos], 1, ContentLength - Pos, In); });
 if (Read == 0) {

Looks like there are unrelated formatting changes in a couple of files



Comment at: clang-tools-extra/clangd/Selection.cpp:350
   }
+  /* fall through and treat as part of the macro body */
 }

FWIW I find this one *much* less clear :-(
I'd personally prefer to ignore the rule in this case, but curious what others 
think


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113892

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 387290.
carlosgalvezp added a comment.

Fix refactoring bug.


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

https://reviews.llvm.org/D113847

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -45,18 +45,13 @@
   std::vector EnabledDiagnosticAliases;
 };
 
-/// Contains displayed and ignored diagnostic counters for a ClangTidy
-/// run.
+/// Contains displayed and ignored diagnostic counters for a ClangTidy run.
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
-
-  unsigned ErrorsDisplayed;
-  unsigned ErrorsIgnoredCheckFilter;
-  unsigned ErrorsIgnoredNOLINT;
-  unsigned ErrorsIgnoredNonUserCode;
-  unsigned ErrorsIgnoredLineFilter;
+  unsigned ErrorsDisplayed = 0;
+  unsigned ErrorsIgnoredCheckFilter = 0;
+  unsigned ErrorsIgnoredNOLINT = 0;
+  unsigned ErrorsIgnoredNonUserCode = 0;
+  unsigned ErrorsIgnoredLineFilter = 0;
 
   unsigned errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -45,18 +45,13 @@
   std::vector EnabledDiagnosticAliases;
 };
 
-/// Contains displayed and ignored diagnostic counters for a ClangTidy
-/// run.
+/// Contains displayed and ignored diagnostic counters for a ClangTidy run.
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
-
-  unsigned ErrorsDisplayed;
-  unsigned ErrorsIgnoredCheckFilter;
-  unsigned ErrorsIgnoredNOLINT;
-  unsigned ErrorsIgnoredNonUserCode;
-  unsigned ErrorsIgnoredLineFilter;
+  unsigned ErrorsDisplayed = 0;
+  unsigned ErrorsIgnoredCheckFilter = 0;
+  unsigned ErrorsIgnoredNOLINT = 0;
+  unsigned ErrorsIgnoredNonUserCode = 0;
+  unsigned ErrorsIgnoredLineFilter = 0;
 
   unsigned errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D113779#3130853 , @paulwalker-arm 
wrote:

> Rather than adding connivence options after the fact what about allowing 
> `-march=` to be specified multiple times? The first must be the usual format 
> with later ones required to start with `+`.  The defined parsing behaviour 
> would be as if there was a single `-march` instance positioned at the first 
> occurrence but containing the value of all instances when combined from left 
> to right.  For example `-march=armv8.4-a .. -march=+nofp16` or perhaps 
> `+=` syntax like  `-march=armv8.4-a .. -march+=nofp16+nosve` is more 
> intuitive?

I think that would be a convenient option which wouldn't require us to add a 
lot of new options, which would be quite cumbersome. But to address the main 
issues (providing a purely additive way to specify features) I think we would 
need to allow `-march=+feature`, without any preceding `-march=armvXXX`.

In D113779#3131569 , @manojgupta 
wrote:

> Yes, the current approach of "-march=+feature" is terrible and does not 
> work with developers who want flexibility of features. This being pitched as 
> a feature imo is akin to promoting a design bug as a feature. 
> Any additive or subtractive alternative is welcome.

I wouldn't go so far as to call the current `-march` handling terrible, but I 
think there are valid use cases that cannot be addressed with it, as per the 
current discussion.  As you said, providing some way specify features 
additively without also committing to a specific architecture version would be 
desirable for our users IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[clang] f0bc7d2 - [analyzer] Fix region cast between the same types with different qualifiers.

[Denys Petrov via cfe-commits]

Author: Denys Petrov
Date: 2021-11-15T19:23:00+02:00
New Revision: f0bc7d24882ab84f6398a1ea614dd0bf6e2a1085

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

LOG: [analyzer] Fix region cast between the same types with different 
qualifiers.

Summary: Specifically, this fixes the case when we get an access to array 
element through the pointer to element. This covers several FIXME's. in 
https://reviews.llvm.org/D111654.
Example:
  const int arr[4][2];
  const int *ptr = arr[1]; // Fixes this.
The issue is that `arr[1]` is `int*` (&Element{Element{glob_arr5,1 
S64b,int[2]},0 S64b,int}), and `ptr` is `const int*`. We don't take qualifiers 
into account. Consequently, we doesn't match the types as the same ones.

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

Added: 


Modified: 
clang/lib/StaticAnalyzer/Core/Store.cpp
clang/test/Analysis/initialization.cpp

Removed: 




diff  --git a/clang/lib/StaticAnalyzer/Core/Store.cpp 
b/clang/lib/StaticAnalyzer/Core/Store.cpp
index 97b8b9d3d07a7..3cc0cd224d7a4 100644
--- a/clang/lib/StaticAnalyzer/Core/Store.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -96,18 +96,24 @@ Optional StoreManager::castRegion(const 
MemRegion *R,
   // already be handled.
   QualType PointeeTy = CastToTy->getPointeeType();
   QualType CanonPointeeTy = Ctx.getCanonicalType(PointeeTy);
+  CanonPointeeTy = CanonPointeeTy.getLocalUnqualifiedType();
 
   // Handle casts to void*.  We just pass the region through.
-  if (CanonPointeeTy.getLocalUnqualifiedType() == Ctx.VoidTy)
+  if (CanonPointeeTy == Ctx.VoidTy)
 return R;
 
-  // Handle casts from compatible types.
-  if (R->isBoundable())
+  const auto IsSameRegionType = [&Ctx](const MemRegion *R, QualType OtherTy) {
 if (const auto *TR = dyn_cast(R)) {
   QualType ObjTy = Ctx.getCanonicalType(TR->getValueType());
-  if (CanonPointeeTy == ObjTy)
-return R;
+  if (OtherTy == ObjTy.getLocalUnqualifiedType())
+return true;
 }
+return false;
+  };
+
+  // Handle casts from compatible types.
+  if (R->isBoundable() && IsSameRegionType(R, CanonPointeeTy))
+return R;
 
   // Process region cast according to the kind of the region being cast.
   switch (R->getKind()) {
@@ -174,16 +180,11 @@ Optional 
StoreManager::castRegion(const MemRegion *R,
   CharUnits off = rawOff.getOffset();
 
   if (off.isZero()) {
-// Edge case: we are at 0 bytes off the beginning of baseR.  We
-// check to see if type we are casting to is the same as the base
-// region.  If so, just return the base region.
-if (const auto *TR = dyn_cast(baseR)) {
-  QualType ObjTy = Ctx.getCanonicalType(TR->getValueType());
-  QualType CanonPointeeTy = Ctx.getCanonicalType(PointeeTy);
-  if (CanonPointeeTy == ObjTy)
-return baseR;
-}
-
+// Edge case: we are at 0 bytes off the beginning of baseR. We check to
+// see if the type we are casting to is the same as the type of the 
base
+// region. If so, just return the base region.
+if (IsSameRegionType(baseR, CanonPointeeTy))
+  return baseR;
 // Otherwise, create a new ElementRegion at offset 0.
 return MakeElementRegion(cast(baseR), PointeeTy);
   }

diff  --git a/clang/test/Analysis/initialization.cpp 
b/clang/test/Analysis/initialization.cpp
index 0883678c8e908..e5b94ea7d0a2b 100644
--- a/clang/test/Analysis/initialization.cpp
+++ b/clang/test/Analysis/initialization.cpp
@@ -68,8 +68,7 @@ void glob_invalid_index3() {
 void glob_invalid_index4() {
   const int *ptr = glob_arr4[1];
   int idx = -42;
-  // FIXME: Should warn {{garbage or undefined}}.
-  auto x = ptr[idx]; // no-warning
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
 }
 
 int const glob_arr5[4][2] = {{1}, 3, 4, 5};
@@ -86,16 +85,11 @@ void glob_array_index3() {
 
 void glob_ptr_index2() {
   int const *ptr = glob_arr5[1];
-  // FIXME: Should be TRUE.
-  clang_analyzer_eval(ptr[0] == 3); // expected-warning{{UNKNOWN}}
-  // FIXME: Should be TRUE.
-  clang_analyzer_eval(ptr[1] == 4); // expected-warning{{UNKNOWN}}
-  // FIXME: Should be UNDEFINED.
-  clang_analyzer_eval(ptr[2] == 5); // expected-warning{{UNKNOWN}}
-  // FIXME: Should be UNDEFINED.
-  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{UNKNOWN}}
-  // FIXME: Should be UNDEFINED.
-  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(ptr[0] == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[1] == 4); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[2] == 5); // expected-warning{{UNDEFINED}}
+  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{UNDEFINED}}
+  clang_analyzer_eval(ptr[4] == 0); // expe

[PATCH] D113480: [analyzer] Fix region cast between the same types with different qualifiers.

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf0bc7d24882a: [analyzer] Fix region cast between the same 
types with different qualifiers. (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113480

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

Index: clang/test/Analysis/initialization.cpp
===
--- clang/test/Analysis/initialization.cpp
+++ clang/test/Analysis/initialization.cpp
@@ -68,8 +68,7 @@
 void glob_invalid_index4() {
   const int *ptr = glob_arr4[1];
   int idx = -42;
-  // FIXME: Should warn {{garbage or undefined}}.
-  auto x = ptr[idx]; // no-warning
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
 }
 
 int const glob_arr5[4][2] = {{1}, 3, 4, 5};
@@ -86,16 +85,11 @@
 
 void glob_ptr_index2() {
   int const *ptr = glob_arr5[1];
-  // FIXME: Should be TRUE.
-  clang_analyzer_eval(ptr[0] == 3); // expected-warning{{UNKNOWN}}
-  // FIXME: Should be TRUE.
-  clang_analyzer_eval(ptr[1] == 4); // expected-warning{{UNKNOWN}}
-  // FIXME: Should be UNDEFINED.
-  clang_analyzer_eval(ptr[2] == 5); // expected-warning{{UNKNOWN}}
-  // FIXME: Should be UNDEFINED.
-  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{UNKNOWN}}
-  // FIXME: Should be UNDEFINED.
-  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(ptr[0] == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[1] == 4); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[2] == 5); // expected-warning{{UNDEFINED}}
+  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{UNDEFINED}}
+  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNDEFINED}}
 }
 
 void glob_invalid_index5() {
@@ -106,8 +100,7 @@
 void glob_invalid_index6() {
   int const *ptr = &glob_arr5[1][0];
   int idx = 42;
-  // FIXME: Should warn {{garbage or undefined}}.
-  auto x = ptr[idx]; // no-warning
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
 }
 
 extern const int glob_arr_no_init[10];
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -96,18 +96,24 @@
   // already be handled.
   QualType PointeeTy = CastToTy->getPointeeType();
   QualType CanonPointeeTy = Ctx.getCanonicalType(PointeeTy);
+  CanonPointeeTy = CanonPointeeTy.getLocalUnqualifiedType();
 
   // Handle casts to void*.  We just pass the region through.
-  if (CanonPointeeTy.getLocalUnqualifiedType() == Ctx.VoidTy)
+  if (CanonPointeeTy == Ctx.VoidTy)
 return R;
 
-  // Handle casts from compatible types.
-  if (R->isBoundable())
+  const auto IsSameRegionType = [&Ctx](const MemRegion *R, QualType OtherTy) {
 if (const auto *TR = dyn_cast(R)) {
   QualType ObjTy = Ctx.getCanonicalType(TR->getValueType());
-  if (CanonPointeeTy == ObjTy)
-return R;
+  if (OtherTy == ObjTy.getLocalUnqualifiedType())
+return true;
 }
+return false;
+  };
+
+  // Handle casts from compatible types.
+  if (R->isBoundable() && IsSameRegionType(R, CanonPointeeTy))
+return R;
 
   // Process region cast according to the kind of the region being cast.
   switch (R->getKind()) {
@@ -174,16 +180,11 @@
   CharUnits off = rawOff.getOffset();
 
   if (off.isZero()) {
-// Edge case: we are at 0 bytes off the beginning of baseR.  We
-// check to see if type we are casting to is the same as the base
-// region.  If so, just return the base region.
-if (const auto *TR = dyn_cast(baseR)) {
-  QualType ObjTy = Ctx.getCanonicalType(TR->getValueType());
-  QualType CanonPointeeTy = Ctx.getCanonicalType(PointeeTy);
-  if (CanonPointeeTy == ObjTy)
-return baseR;
-}
-
+// Edge case: we are at 0 bytes off the beginning of baseR. We check to
+// see if the type we are casting to is the same as the type of the base
+// region. If so, just return the base region.
+if (IsSameRegionType(baseR, CanonPointeeTy))
+  return baseR;
 // Otherwise, create a new ElementRegion at offset 0.
 return MakeElementRegion(cast(baseR), PointeeTy);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113306: [PowerPC] Allow MMA built-ins to accept non-void pointers and arrays

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM. Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113306

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:61-64
-  unsigned errorsIgnored() const {
+  int errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }

carlosgalvezp wrote:
> aaron.ballman wrote:
> > carlosgalvezp wrote:
> > > aaron.ballman wrote:
> > > > The type changes overflow here from well-defined behavior to undefined 
> > > > behavior. I don't think that's a serious concern (we have error limits 
> > > > already), but in general, I don't think we should be changing types 
> > > > like this without stronger rationale. This particular bit feels like 
> > > > churn without benefit -- what do we gain by introducing the possibility 
> > > > for UB here? (I'm not strongly opposed, but I get worried when 
> > > > well-defined code turns into potentially undefined code in the name of 
> > > > an NFC cleanup.)
> > > Do we have coding guidelines for when to use signed vs unsigned? I have 
> > > quite ugly past experiences using unsigned types for arithmetic and as 
> > > "counters" so these kinds of things really catch my eye quickly. This 
> > > goes in line with e.g. the [[ 
> > > https://google.github.io/styleguide/cppguide.html#Integer_Types | Google 
> > > style guide ]].
> > > 
> > > Yes, you are correct that signed int overflow is UB, but would that then 
> > > be an argument for banning signed ints in the codebase at all? IMO what's 
> > > more important is to know the domain in which this is being applied. Do 
> > > we expect people to have more than 2^31 linter warnings? If we do, would 
> > > 2^32 really solve the problem, or just delay it? Maybe the proper 
> > > solution is to go to 64-bit ints if that's an expected use case.
> > > 
> > > Anyway if this is a concern I'm happy to revert and take the discussion 
> > > outside the patch. We used to be over-defensive in this topic as well (UB 
> > > is not to be taken lightly) but realized that perhaps the problem lies 
> > > somewhere else.
> > > Do we have coding guidelines for when to use signed vs unsigned?
> > 
> > We don't, and I don't think we could even if we wanted to. There are times 
> > when unsigned is correct and there are times when signed is correct, and no 
> > blanket rule covers all the cases.
> > 
> > > Yes, you are correct that signed int overflow is UB, but would that then 
> > > be an argument for banning signed ints in the codebase at all? 
> > 
> > No. My point about the change to UB wasn't "I think this is dangerous" -- 
> > as I mentioned, we have limits on the number of errors that can be emitted, 
> > so I don't believe we can hit the UB in practice. It's more that changing 
> > types is rarely an NFC change without further validation and it wasn't 
> > clear if this validation had happened. Changing types is a nontrivial (and 
> > rarely NFC) change in C++.
> > 
> > > Anyway if this is a concern I'm happy to revert and take the discussion 
> > > outside the patch. We used to be over-defensive in this topic as well (UB 
> > > is not to be taken lightly) but realized that perhaps the problem lies 
> > > somewhere else.
> > 
> > Personally, I'm not super motivated to see a change here, but I'm also not 
> > opposed to it. It mostly just feels like churn. If we're solving a problem, 
> > I'm happy to see the changes, but we should call out what problem we're 
> > solving in the patch summary. But this seems more a style choice and I 
> > certainly wouldn't want to see these changes used as a precedent to go 
> > switch more unsigned ints to be signed ints in the name of style.
> Thanks, I see how this change is not as NFC as I thought would be. Reverting!
> But this seems more a style choice

PS: IMHO signed vs unsigned is not just a style choice. Using unsigned types 
for the wrong purpose (e.g. doing arithmetic) can lead to nasty logical bugs, 
especially when implicit conversions happen under the hood:

https://godbolt.org/z/5qox7b1nW

The above code is obvious, but in a real-world case you might not keep in the 
back of your head that you are operating with an "unsigned" variable in the 
middle of a 100-lines long function.

In the end, it's all about where you place the "singularity". With unsigned 
types, underflow happens around 0. With signed types, UB happens at 
min()/max(). In real-world applications, we usually code much more often around 
0 than around min/max.

That's my 2 cents, don't mean to change/promote any guidelines. If you think 
this is a topic of interest I can bring it to the mailing list, otherwise I'll 
just drop it :)


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

https://reviews.llvm.org/D113847

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

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 387304.
carlosgalvezp added a comment.

- Replace composition with private inheritance.
- Fix namespaces.

Public inheritance is not possible due to the base class' function being 
const-qualified; the derived class cannot be const since it updates state 
(unless we go into mutable territory, which I think goes in detriment of 
readability). What do you think?


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

https://reviews.llvm.org/D113422

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/GlobList.cpp
  clang-tools-extra/clang-tidy/GlobList.h
  clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
@@ -4,15 +4,20 @@
 namespace clang {
 namespace tidy {
 
-TEST(GlobList, Empty) {
-  GlobList Filter("");
+template  struct GlobListTest : public ::testing::Test {};
+
+using GlobListTypes = ::testing::Types;
+TYPED_TEST_SUITE(GlobListTest, GlobListTypes);
+
+TYPED_TEST(GlobListTest, Empty) {
+  TypeParam Filter("");
 
   EXPECT_TRUE(Filter.contains(""));
   EXPECT_FALSE(Filter.contains("aaa"));
 }
 
-TEST(GlobList, Nothing) {
-  GlobList Filter("-*");
+TYPED_TEST(GlobListTest, Nothing) {
+  TypeParam Filter("-*");
 
   EXPECT_FALSE(Filter.contains(""));
   EXPECT_FALSE(Filter.contains("a"));
@@ -21,8 +26,8 @@
   EXPECT_FALSE(Filter.contains("*"));
 }
 
-TEST(GlobList, Everything) {
-  GlobList Filter("*");
+TYPED_TEST(GlobListTest, Everything) {
+  TypeParam Filter("*");
 
   EXPECT_TRUE(Filter.contains(""));
   EXPECT_TRUE(Filter.contains(""));
@@ -31,8 +36,8 @@
   EXPECT_TRUE(Filter.contains("*"));
 }
 
-TEST(GlobList, OneSimplePattern) {
-  GlobList Filter("aaa");
+TYPED_TEST(GlobListTest, OneSimplePattern) {
+  TypeParam Filter("aaa");
 
   EXPECT_TRUE(Filter.contains("aaa"));
   EXPECT_FALSE(Filter.contains(""));
@@ -41,8 +46,8 @@
   EXPECT_FALSE(Filter.contains("bbb"));
 }
 
-TEST(GlobList, TwoSimplePatterns) {
-  GlobList Filter("aaa,bbb");
+TYPED_TEST(GlobListTest, TwoSimplePatterns) {
+  TypeParam Filter("aaa,bbb");
 
   EXPECT_TRUE(Filter.contains("aaa"));
   EXPECT_TRUE(Filter.contains("bbb"));
@@ -52,11 +57,11 @@
   EXPECT_FALSE(Filter.contains(""));
 }
 
-TEST(GlobList, PatternPriority) {
+TYPED_TEST(GlobListTest, PatternPriority) {
   // The last glob that matches the string decides whether that string is
   // included or excluded.
   {
-GlobList Filter("a*,-aaa");
+TypeParam Filter("a*,-aaa");
 
 EXPECT_FALSE(Filter.contains(""));
 EXPECT_TRUE(Filter.contains("a"));
@@ -65,7 +70,7 @@
 EXPECT_TRUE(Filter.contains(""));
   }
   {
-GlobList Filter("-aaa,a*");
+TypeParam Filter("-aaa,a*");
 
 EXPECT_FALSE(Filter.contains(""));
 EXPECT_TRUE(Filter.contains("a"));
@@ -75,15 +80,16 @@
   }
 }
 
-TEST(GlobList, WhitespacesAtBegin) {
-  GlobList Filter("-*,   a.b.*");
+TYPED_TEST(GlobListTest, WhitespacesAtBegin) {
+  TypeParam Filter("-*,   a.b.*");
 
   EXPECT_TRUE(Filter.contains("a.b.c"));
   EXPECT_FALSE(Filter.contains("b.c"));
 }
 
-TEST(GlobList, Complex) {
-  GlobList Filter("*,-a.*, -b.*, \r  \n  a.1.* ,-a.1.A.*,-..,-...,-..+,-*$, -*qwe* ");
+TYPED_TEST(GlobListTest, Complex) {
+  TypeParam Filter(
+  "*,-a.*, -b.*, \r  \n  a.1.* ,-a.1.A.*,-..,-...,-..+,-*$, -*qwe* ");
 
   EXPECT_TRUE(Filter.contains("aaa"));
   EXPECT_TRUE(Filter.contains("qqq"));
Index: clang-tools-extra/clang-tidy/GlobList.h
===
--- clang-tools-extra/clang-tidy/GlobList.h
+++ clang-tools-extra/clang-tidy/GlobList.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Regex.h"
 
@@ -47,7 +48,22 @@
   SmallVector Items;
 };
 
-} // end namespace tidy
-} // end namespace clang
+/// A \p GlobList that caches search results, so that search is performed only
+/// once for the same query.
+class CachedGlobList : private GlobList {
+public:
+  /// \see GlobList::GlobList
+  CachedGlobList(StringRef Globs, bool KeepNegativeGlobs = true);
+
+  /// \see GlobList::contains
+  bool contains(StringRef S);
+
+private:
+  enum Tristate { None, Yes, No };
+  llvm::StringMap Cache;
+};
+
+} // namespace tidy
+} // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GLOBLIST_H
Index: clang-tools-extra/clang-tidy/GlobList.cpp
===
--- clang-tools-extra/clang-tidy/GlobList.cpp
+++ clang-tools-extra/clang-tidy/GlobList.cpp
@@ -9,8 +9,8 @@
 #include "GlobList.h"
 #include "llvm/ADT/SmallString.h"
 
-using namespace c

[PATCH] D107994: Making the code compliant to the documentation about Floating Point support default values for C/C++. FPP-MODEL=PRECISE enables FFP-CONTRACT (FMA is enabled).

[Warren Ristow via Phabricator via cfe-commits]

wristow added a comment.

In D107994#3131268 , @zahiraam wrote:

> In D107994#3130494 , @wristow wrote:
>
>> The Release Note change here says:
>>
>>   Floating Point Support in Clang
>>   ---
>>   - The -ffp-model=precise now implies -ffp-contract=on rather than
>> -ffp-contract=fast, and the documentation of these features has been
>> clarified. Previously, the documentation claimed that -ffp-model=precise 
>> was
>> the default, but this was incorrect because the precise model implied
>> -ffp-contract=fast, whereas the default behavior is -ffp-contract=on.
>> -ffp-model=precise is now exactly the default mode of the compiler.
>>
>> Unless I'm missing something, there is a related change here that I think 
>> should be more overtly noted (given the discussions in this review, I 
>> //think// this additional change is understood/expected, but I'm surprised 
>> it's not pointed out explicitly -- so maybe I'm misunderstanding).
>>
>> Specifically, this commit explicitly sets `-ffp-contract=on` in the default 
>> mode (which is what the documentation said, and continues to say).  But 
>> previously, there wasn't //any// explicit setting of `-ffp-contract` by 
>> default (and I think that lack of an explicit setting, was equivalent to 
>> `-ffp-contract=off`).
>> ...
>
> @wristow Are you suggesting a change of wording in the ReleaseNotes?

Yes @zahiraam, either a modification to what was written, or an additional 
separate point.  The critical user-visible change from this commit (AFAICS) is 
that previously the default behavior was `-ffp-contract=off`, whereas now the 
default behavior is `-ffp-contract=on`.  The ReleaseNote as written implies 
that the FMA situation //was// `-ffp-contract=fast`, and it has been changed to 
`-ffp-contract=on`, and either of those modes would result in FMA being done 
for cases like:

  float foo(float a, float b, float c) {
return a * b + c;
  }

In short, the current ReleaseNote implies that FMA would be used by default, 
for cases like the above (because `-ffp-contract=on` was the default) -- but 
this isn't true.  (The current ReleaseNote also clarifies that if 
`-ffp-model=precise` were explicitly specified, then it previously would set 
`-ffp-contract=fast` (which was not what the documentation said) and now it 
sets `-ffp-contract=on` (which //is// what the documentation says) -- this 
aspect is correct.)

I think the ReleaseNote should clarify that the result of this change is that 
previously, the default behavior was equivalent to `-ffp-contract=off` (and so 
no FMA would be done by default), but this commit makes the default behavior 
`-ffp-contract=on` (so FMA is enabled by default, even at `-O0`).  The 
documentation indicates that the default is `-ffp-contract=on`, so this change 
makes the compiler behavior consistent with the documentation, with respect to 
the `-ffp-contact` switch.  It also makes it consistent with the documentation 
for the `-ffp-model` switch.

A possible new wording is below (maybe this is too verbose, so making it more 
concise is fine too, from my POV):

  Floating Point Support in Clang
  ---
  - The default setting of FP contraction (FMA) is now -ffp-contract=on (for
languages other than CUDA/HIP).  This is consistent with the documentation.
Previously, the default behavior was equivalent to -ffp-contract=off, so
the old behavior was that FMA would not be used by default for code like:
float foo(float a, float b, float c) {
  return a * b + c;
}
Whereas now, FMA will be used in the above code, even when optimization
is off.
  
Related to this, the switch -ffp-model=precise now implies -ffp-contract=on
rather than -ffp-contract=fast, and the documentation of these features has
been clarified.  Previously, the documentation claimed that
-ffp-model=precise was the default, but this was incorrect because the
precise model implied -ffp-contract=fast, whereas the (now corrected)
default behavior is -ffp-contract=on.
-ffp-model=precise is now exactly the default mode of the compiler.


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

https://reviews.llvm.org/D107994

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

[clang] 0b9d3a6 - [analyzer][NFC] Separate CallDescription from CallEvent

[Balazs Benics via cfe-commits]

Author: Balazs Benics
Date: 2021-11-15T19:10:46+01:00
New Revision: 0b9d3a6e53e6c6488b531f3c8c281b485ca3b14a

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

LOG: [analyzer][NFC] Separate CallDescription from CallEvent

`CallDescriptions` deserve its own translation unit.
This patch simply moves the corresponding parts.
Also includes the `CallDescription.h` where it's necessary.

Reviewed By: martong, xazax.hun, Szelethus

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

Added: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
clang/lib/StaticAnalyzer/Core/CallDescription.cpp

Modified: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp
clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
clang/lib/StaticAnalyzer/Core/CMakeLists.txt
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp
clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn

Removed: 




diff  --git 
a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h 
b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
new file mode 100644
index 0..9148dcaa87544
--- /dev/null
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -0,0 +1,121 @@
+//===- CallDescription.h - function/method call matching   --*- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+/// \file This file defines a generic mechanism for matching for function and
+/// method calls of C, C++, and Objective-C languages. Instances of these
+/// classes are frequently used together with the CallEvent classes.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CALLDESCRIPTION_H
+#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CALLDESCRIPTION_H
+
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
+#include 
+
+namespace clang {
+class IdentifierInfo;
+} // namespace clang
+
+namespace clang {
+namespace ento {
+
+enum CallDescriptionFlags : int {
+  /// Describes a C standard function that is sometimes implemented as a macro
+  /// that expands to a compiler builtin with some __builtin prefix.
+  /// The builtin may as well have a few extra arguments on top of the 
requested
+  /// number of arguments.
+  CDF_MaybeBuiltin = 1 << 0,
+};
+
+/// This class represents a description of a function call using the number of
+/// arguments and the name of the function.
+class CallDescription {
+  friend class CallEvent;
+  mutable Optional II;
+  // The list of the qualified names used to identify the specified CallEvent,
+  // e.g. "{a, b}" represent the qualified names, like "a::b".

[PATCH] D113587: [analyzer][NFC] Separate CallDescription from CallEvent

[Balázs Benics via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0b9d3a6e53e6: [analyzer][NFC] Separate CallDescription from 
CallEvent (authored by steakhal).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113587

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/CallDescription.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
  clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
  clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
  llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Core/BUILD.gn
@@ -20,6 +20,7 @@
 "BlockCounter.cpp",
 "BugReporter.cpp",
 "BugReporterVisitors.cpp",
+"CallDescription.cpp",
 "CallEvent.cpp",
 "Checker.cpp",
 "CheckerContext.cpp",
Index: clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
@@ -14,6 +14,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -12,6 +12,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
Index: clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
===
--- clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
+++ clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
@@ -8,6 +8,7 @@
 
 #include "CheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAn

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 387318.
avogelsgesang added a comment.

Update docs to also mention `container.find() != container.end()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+  void *find(const Key &K);
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+} // namespace std
+
+using namespace std;
+
+// Check that we detect various common ways to check for containment
+int testDifferentCheckTypes(map &MyMap) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-containment
+int testNegativeChecks(map &MyMap) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(map &M, unordered_set &US, set &S, multimap &MM) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use `contains` instead of `count` t

[PATCH] D113917: Add infrastructure to support matcher names.

[James King via Phabricator via cfe-commits]

jcking1034 created this revision.
jcking1034 added reviewers: ymandel, tdl-g, aaron.ballman.
jcking1034 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This contains changes to the AST Matcher infrastructure in order to provide
contextual information about each matcher. This information can be useful for
tasks such as performing introspection on matchers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113917

Files:
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/include/clang/ASTMatchers/ASTMatchersMacros.h
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp

Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -115,15 +115,19 @@
 template 
 class VariadicMatcher : public DynMatcherInterface {
 public:
-  VariadicMatcher(std::vector InnerMatchers)
-  : InnerMatchers(std::move(InnerMatchers)) {}
+  VariadicMatcher(std::string MatcherName,
+  std::vector InnerMatchers)
+  : MatcherName(MatcherName), InnerMatchers(std::move(InnerMatchers)) {}
 
   bool dynMatches(const DynTypedNode &DynNode, ASTMatchFinder *Finder,
   BoundNodesTreeBuilder *Builder) const override {
 return Func(DynNode, Finder, Builder, InnerMatchers);
   }
 
+  std::string getName() const override { return MatcherName; }
+
 private:
+  const std::string MatcherName;
   std::vector InnerMatchers;
 };
 
@@ -144,11 +148,39 @@
 return InnerMatcher->TraversalKind();
   }
 
+  std::string getName() const override { return InnerMatcher->getName(); }
+
 private:
   const std::string ID;
   const IntrusiveRefCntPtr InnerMatcher;
 };
 
+/// A matcher that specifies a particular name.
+///
+/// The name provided to the constructor overrides any name that may be
+/// specified by the `InnerMatcher`.
+class NameMatcherImpl : public DynMatcherInterface {
+public:
+  NameMatcherImpl(std::string _MatcherName,
+  IntrusiveRefCntPtr InnerMatcher)
+  : MatcherName(_MatcherName), InnerMatcher(std::move(InnerMatcher)) {}
+
+  bool dynMatches(const DynTypedNode &DynNode, ASTMatchFinder *Finder,
+  BoundNodesTreeBuilder *Builder) const override {
+return InnerMatcher->dynMatches(DynNode, Finder, Builder);
+  }
+
+  std::string getName() const override { return MatcherName; }
+
+  llvm::Optional TraversalKind() const override {
+return InnerMatcher->TraversalKind();
+  }
+
+private:
+  const std::string MatcherName;
+  const IntrusiveRefCntPtr InnerMatcher;
+};
+
 /// A matcher that always returns true.
 class TrueMatcherImpl : public DynMatcherInterface {
 public:
@@ -158,6 +190,8 @@
   BoundNodesTreeBuilder *) const override {
 return true;
   }
+
+  std::string getName() const override { return "TrueMatcher"; }
 };
 
 /// A matcher that specifies a particular \c TraversalKind.
@@ -180,6 +214,8 @@
 return TK;
   }
 
+  std::string getName() const override { return InnerMatcher->getName(); }
+
 private:
   clang::TraversalKind TK;
   IntrusiveRefCntPtr InnerMatcher;
@@ -219,31 +255,31 @@
   RestrictKind =
   ASTNodeKind::getMostDerivedType(RestrictKind, IM.RestrictKind);
 }
-return DynTypedMatcher(
-SupportedKind, RestrictKind,
-new VariadicMatcher(std::move(InnerMatchers)));
+return DynTypedMatcher(SupportedKind, RestrictKind,
+   new VariadicMatcher(
+   "allOf", std::move(InnerMatchers)));
 
   case VO_AnyOf:
-return DynTypedMatcher(
-SupportedKind, RestrictKind,
-new VariadicMatcher(std::move(InnerMatchers)));
+return DynTypedMatcher(SupportedKind, RestrictKind,
+   new VariadicMatcher(
+   "anyOf", std::move(InnerMatchers)));
 
   case VO_EachOf:
-return DynTypedMatcher(
-SupportedKind, RestrictKind,
-new VariadicMatcher(std::move(InnerMatchers)));
+return DynTypedMatcher(SupportedKind, RestrictKind,
+   new VariadicMatcher(
+   "eachOf", std::move(InnerMatchers)));
 
   case VO_Optionally:
 return DynTypedMatcher(SupportedKind, RestrictKind,
new VariadicMatcher(
-   std::move(InnerMatchers)));
+   "optionally", std::move(InnerMatchers)));
 
   case VO_UnaryNot:
 // FIXME: Implement the Not operator to take a single matcher instead of a
 // vector.
 return DynTypedMatcher(
 SupportedKind, RestrictKind,
-new VariadicMatcher(std::move(InnerMatchers)));
+new VariadicMatcher("not", std::move(InnerMatchers)));
   }
   llvm_unreachable("Invalid Op value.");
 }
@@ -263,6 +299,14 @@
   return Copy;
 }
 
+DynTypedMatcher
+DynTypedMatc

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

@xazax.hun / @whisperity could you review this change for me, please :) 
Or, alternatively: can you direct me to somebody who could review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D113917: Add infrastructure to support matcher names.

[James King via Phabricator via cfe-commits]

jcking1034 added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1021
 
+  std::string getName() const override {
+return "HasOverloadedOperatorNameMatcher";

Here and below, we have the option to use the name of the matcher (for this, we 
could use "hasOverloadedOperatorName" instead). However, some of these classes 
are used in multiple matchers. For example, `HasOverloadedOperatorNameMatcher` 
is used in both `hasOverloadedOperatorName` and `hasAnyOverloadedOperatorName`, 
which I'm a bit concerned about.



Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1409
   return BindableMatcher(
-  makeAllOfComposite(InnerMatchers).template dynCastTo());
+  makeAllOfComposite(InnerMatchers).template dynCastTo(),
+  makeMatcherNameFromType());

@ymandel suggested an alternate implementation where we instead pass the 
matcher name to `makeAllOfComposite`, which then passes the name to 
`constructVariadic`, to avoid making changes to `BindableMatcher`. I may look 
into this again, but my previous attempts to try this approach seemed messy due 
to the fact that these functions are used in a few different places, and 
because `makeAllOfComposite` handles cases with 0 or 1 inner matchers without 
constructing a variadic matcher.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113917

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

[PATCH] D112648: [clang-tidy] Improve fix-its for `modernize-pass-by-value` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added subscribers: ymandel, whisperity, aaron.ballman.
avogelsgesang added a comment.

@ymandel,  @whisperity, @aaron.ballman could one of you review this/point me in 
the direction of a good reviewer for this change?
(Sorry for the spam - I am new to the LLVM project, and I guess I still have to 
learn how to navigate Phabricator/find the correct reviewers for my changes)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:61-64
-  unsigned errorsIgnored() const {
+  int errorsIgnored() const {
 return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }

carlosgalvezp wrote:
> carlosgalvezp wrote:
> > aaron.ballman wrote:
> > > carlosgalvezp wrote:
> > > > aaron.ballman wrote:
> > > > > The type changes overflow here from well-defined behavior to 
> > > > > undefined behavior. I don't think that's a serious concern (we have 
> > > > > error limits already), but in general, I don't think we should be 
> > > > > changing types like this without stronger rationale. This particular 
> > > > > bit feels like churn without benefit -- what do we gain by 
> > > > > introducing the possibility for UB here? (I'm not strongly opposed, 
> > > > > but I get worried when well-defined code turns into potentially 
> > > > > undefined code in the name of an NFC cleanup.)
> > > > Do we have coding guidelines for when to use signed vs unsigned? I have 
> > > > quite ugly past experiences using unsigned types for arithmetic and as 
> > > > "counters" so these kinds of things really catch my eye quickly. This 
> > > > goes in line with e.g. the [[ 
> > > > https://google.github.io/styleguide/cppguide.html#Integer_Types | 
> > > > Google style guide ]].
> > > > 
> > > > Yes, you are correct that signed int overflow is UB, but would that 
> > > > then be an argument for banning signed ints in the codebase at all? IMO 
> > > > what's more important is to know the domain in which this is being 
> > > > applied. Do we expect people to have more than 2^31 linter warnings? If 
> > > > we do, would 2^32 really solve the problem, or just delay it? Maybe the 
> > > > proper solution is to go to 64-bit ints if that's an expected use case.
> > > > 
> > > > Anyway if this is a concern I'm happy to revert and take the discussion 
> > > > outside the patch. We used to be over-defensive in this topic as well 
> > > > (UB is not to be taken lightly) but realized that perhaps the problem 
> > > > lies somewhere else.
> > > > Do we have coding guidelines for when to use signed vs unsigned?
> > > 
> > > We don't, and I don't think we could even if we wanted to. There are 
> > > times when unsigned is correct and there are times when signed is 
> > > correct, and no blanket rule covers all the cases.
> > > 
> > > > Yes, you are correct that signed int overflow is UB, but would that 
> > > > then be an argument for banning signed ints in the codebase at all? 
> > > 
> > > No. My point about the change to UB wasn't "I think this is dangerous" -- 
> > > as I mentioned, we have limits on the number of errors that can be 
> > > emitted, so I don't believe we can hit the UB in practice. It's more that 
> > > changing types is rarely an NFC change without further validation and it 
> > > wasn't clear if this validation had happened. Changing types is a 
> > > nontrivial (and rarely NFC) change in C++.
> > > 
> > > > Anyway if this is a concern I'm happy to revert and take the discussion 
> > > > outside the patch. We used to be over-defensive in this topic as well 
> > > > (UB is not to be taken lightly) but realized that perhaps the problem 
> > > > lies somewhere else.
> > > 
> > > Personally, I'm not super motivated to see a change here, but I'm also 
> > > not opposed to it. It mostly just feels like churn. If we're solving a 
> > > problem, I'm happy to see the changes, but we should call out what 
> > > problem we're solving in the patch summary. But this seems more a style 
> > > choice and I certainly wouldn't want to see these changes used as a 
> > > precedent to go switch more unsigned ints to be signed ints in the name 
> > > of style.
> > Thanks, I see how this change is not as NFC as I thought would be. 
> > Reverting!
> > But this seems more a style choice
> 
> PS: IMHO signed vs unsigned is not just a style choice. Using unsigned types 
> for the wrong purpose (e.g. doing arithmetic) can lead to nasty logical bugs, 
> especially when implicit conversions happen under the hood:
> 
> https://godbolt.org/z/5qox7b1nW
> 
> The above code is obvious, but in a real-world case you might not keep in the 
> back of your head that you are operating with an "unsigned" variable in the 
> middle of a 100-lines long function.
> 
> In the end, it's all about where you place the "singularity". With unsigned 
> types, underflow happens around 0. With signed types, UB happens at 
> min()/max(). In real-world applications, we usually code much more often 
> around 0 than around min/max.
> 
> That's my 2 cents, don't mean to change/promote any guidelines. If you think 
> this is a topic of interest I can bring it to the mailing list, otherwise 
> I'll just drop it :)
> That's my 2