[PATCH] D90123: [Sema] Improve notes for value category mismatch in overloading

2020-10-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:4927 // Note that the function case is not possible here. -if (DeclType->isRValueReferenceType() && LValRefType) { - // FIXME: This is the wrong BadConversionSequence. The problem is bin

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

2020-10-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:1275 +const ValueDecl *VD = LP->clangDecl(); +return VD->isDefinedOutsideFunctionOrMethod(); + } aaron.ballman wrote: > Hmm, I've not seen that function used a whole lot

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

2020-10-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87-89 + void test4() { +MutexLock lock(&mu); // expected-warning {{acquiring mutex 'mu' requires negative capability '!mu'}} + } lebedev.ri wrote: > aaronpuc

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

2020-10-25 Thread Aaron Puchert 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 rG5250a03a9959: Thread safety analysis: Consider global variables in scope (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D90129: Source location for -Wignored-qualifiers on lambda trailing return type

2020-10-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. aaronpuchert requested review of this revision. We don't have a source location for trailing return types at the moment, and the current f

[PATCH] D90129: Source location for -Wignored-qualifiers on lambda trailing return type

2020-10-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:3092-3094 +SourceLocation Loc = D.getIdentifierLoc(); +if (Loc.isInvalid()) + Loc = FTI.getRParenLoc(); I'm open to always using the `RParenLoc`. Repository: rG LLVM Gith

[PATCH] D90129: Source location for -Wignored-qualifiers on lambda trailing return type

2020-10-27 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert updated this revision to Diff 301161. aaronpuchert marked an inline comment as done. aaronpuchert added a comment. Collect location of a trailing return type in the parser, use that for the warning. I resisted clang-format's urge to reflow the large parameter lists, I hope that's t

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

2020-10-28 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Can't really add anything to the discussion between @Quuxplusone and the author, just a few comments about the test. Comment at: clang/test/SemaCXX/implicitly-movable.cpp:1 +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify %s +

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

2020-10-28 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/test/SemaCXX/implicitly-movable.cpp:14 + +private: + A(const A &); aaronpuchert wrote: > Is this testing what we want it to test? The private functions are just not > part of the overload set, right? > > I

[PATCH] D90129: Better source location for -Wignored-qualifiers on trailing return types

2020-10-28 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert marked an inline comment as done. aaronpuchert added inline comments. Comment at: clang/include/clang/Sema/DeclSpec.h:1509 +SourceLocation getTrailingReturnTypeLoc() const { + return SourceLocation::getFromRawEncoding(TrailingReturnTypeLoc); +} -

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

2020-10-28 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/test/SemaCXX/implicitly-movable.cpp:14 + +private: + A(const A &); aaronpuchert wrote: > Quuxplusone wrote: > > aaronpuchert wrote: > > > Is this testing what we want it to test? The private functions are jus

[PATCH] D90129: Better source location for -Wignored-qualifiers on trailing return types

2020-10-28 Thread Aaron Puchert via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. aaronpuchert marked an inline comment as done. Closed by commit rG5dbccc6c89c0: Better source location for -Wignored-qualifiers on trailing return types (authored by aa

[PATCH] D90129: Better source location for -Wignored-qualifiers on trailing return types

2020-10-28 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert marked an inline comment as done. aaronpuchert added a comment. Added the assertions in a follow-up change rGebfc427bbe08f0c36af9721d5a4e6d3ffe2e4bf5 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

2020-10-29 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D84604#2363134 , @rupprecht wrote: > I'm seeing failures which I think are due to this patch -- I don't have a > nice godbolt repro yet, but it's something like: Yes, that's very likely. > I'm also seeing the same error

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

2020-10-29 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Pushed a fix in rGbbed8cfe80cd27d3a47d877c7608d9be4e487d97 . For now we just consider all static members as inaccessible, so we'll treat them as we did before this change. I have proposed making

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

2020-10-30 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D84604#2363768 , @rupprecht wrote: > I applied D87194 locally and rebuilt the > original source, and not only am I seeing the original issue (also firing on > `DoThings()` when it should

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-10-30 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert planned changes to this revision. aaronpuchert added a comment. Need to rebase this on top of D84604 plus a subsequent fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87194/new/ https://reviews.llv

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

2020-10-30 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D88295#2365474 , @rsmith wrote: > The other is whether implicitly move in a `co_return` statement. In that > case, I think the use of `CES_AsIfByStdMove` is a mistake, and we should be > using `CES_Default` there. I did

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

2020-10-30 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D88295#2365474 , @rsmith wrote: > ... where `X` has a volatile copy constructor and a volatile move > constructor, I think we should produce the warning suggesting use of > `std::move`. If I read this correctly, we'd hav

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-02 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:158 +} + } // end namespace ScopeTest aaron.ballman wrote: > Given that this is touching on declaration contexts, it would also be good to > have some tests chec

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-02 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert updated this revision to Diff 302388. aaronpuchert marked 2 inline comments as done. aaronpuchert added a comment. Rebased on D84604 , included static members, and addressed review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-02 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a subscriber: rupprecht. aaronpuchert added a comment. @rupprecht, maybe you can try it again? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87194/new/ https://reviews.llvm.org/D87194

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

2021-05-13 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Ping @aaron.ballman, also for the parent change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102026/new/ https://reviews.llvm.org/D102026 ___ cfe-commits mailing list cfe-

[PATCH] D102025: Thread safety analysis: Factor out function for merging locks (NFC)

2021-05-13 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:2199 +// Take the exclusive capability to reduce further warnings. +return B.kind() == LK_Exclusive; + } else { aaron.ballman wrote: > The old code was looking at `LDat1.

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

2021-05-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a subscriber: aaron.ballman. aaronpuchert added a comment. @aaron.ballman, what do you think about this? We can't really prevent anyone from writing `.getValueKind() == VK_*Value` instead of `.is*Value()`, so this change will make things consistent only for now. In D100733#26

[PATCH] D100713: [clang] NFC: refactor usage of getDecltypeForParenthesizedExpr

2021-05-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Again, I'm not sure if it helps to use `getDecltypeForParenthesizedExpr` where we don't actually have the `decltype` of a parenthesized expression. It's probably not entirely coincidental, but things aren't defined this way. Comment at: clang/lib

[PATCH] D99455: [AST] Store regular ValueDecl* in BindingDecl

2021-05-20 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Ping @rsmith. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99455/new/ https://reviews.llvm.org/D99455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.

[PATCH] D99455: [AST] Store regular ValueDecl* in BindingDecl

2021-05-20 Thread Aaron Puchert 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 rGa5c2ec96e5f9: [AST] Store regular ValueDecl* in BindingDecl (NFC) (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

2021-05-21 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D100733#2760890 , @mizvekov wrote: > - I still think a new value category is simpler than the two phase overload > resolution it replaces :D Not sure if it's simpler, but it'll produce less confusing results. > - This n

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

2021-05-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D102026#2777438 , @delesley wrote: > The warn/not-warn is consistent with more relaxed handling of managed locks, > but exclusive/shared difference could lead to confusion. Both mechanisms are > sound; they're just not

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

2021-05-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D102026#2779983 , @aaronpuchert wrote: > So this change will allow shared/exclusive - asserted/unmanaged joins until > my follow-up will disallow them again, though because of the > asserted/unmanaged part and not the s

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

2021-05-27 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D102026#2780384 , @delesley wrote: > Thanks for taking the time to discuss things with me. :-) Thank you as well! > Wrt. to the TEST_LOCKED_FUNCTION, I agree that you can simulate the behavior > using Assert and Lock.

[PATCH] D102025: Thread safety analysis: Factor out function for merging locks (NFC)

2021-05-27 Thread Aaron Puchert via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG3d64677c2807: Thread safety analysis: Factor out function for merging locks (NFC) (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

2021-05-27 Thread Aaron Puchert via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. aaronpuchert marked 2 inline comments as done. Closed by commit rGcf0b337c1b1f: Thread safety analysis: Allow exlusive/shared joins for managed and asserted… (authored

[PATCH] D101566: Let -Wweak-template-vtables warn on implicit instantiations

2021-05-27 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D101566#2734948 , @dblaikie wrote: > Makes it hard to justify the complexity in the compiler if it's hard to > justify/support the value of the warning. The complexity for `-Wweak-template-vtables` is just 10 lines of co

[PATCH] D101566: Let -Wweak-template-vtables warn on implicit instantiations

2021-05-27 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D101566#2785271 , @dblaikie wrote: > Right - to remove -Wweak-template-vtable in its entirety. The original > implementation explicitly didn't warn on implicit instantiations and I think > the fact that it warned on expl

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

2021-05-27 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D102026#2785243 , @delesley wrote: > Assert_capability is not a back door. It is supposed to be used only on a > function which does a run-time check: if (!mu_.is_locked()) fail(). Right, although assertions can turn i

[PATCH] D106713: Thread safety analysis: Warn when demoting locks on back edges

2021-09-18 Thread Aaron Puchert 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 rG9b889f826ff5: Thread safety analysis: Warn when demoting locks on back edges (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHAN

[PATCH] D106715: Thread safety analysis: Drop special block handling

2021-09-18 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106715/new/ https://reviews.llvm.org/D106715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D106713: Thread safety analysis: Warn when demoting locks on back edges

2021-09-18 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. @aaron.ballman, since this is reintroducing some warnings after the relaxation in D102026 , should we bring this to Clang 13? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106713/new/

[PATCH] D106715: Thread safety analysis: Drop special block handling

2021-09-20 Thread Aaron Puchert 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 rG6de19ea4b626: Thread safety analysis: Drop special block handling (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D106713: Thread safety analysis: Warn when demoting locks on back edges

2021-09-20 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a subscriber: tstellar. aaronpuchert added a comment. In D106713#3009130 , @aaron.ballman wrote: > In D106713#3007878 , @aaronpuchert > wrote: > >> @aaron.ballman, since this is reintroducing

[PATCH] D106713: Thread safety analysis: Warn when demoting locks on back edges

2021-09-20 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D106713#3009542 , @tstellar wrote: > @aaronpuchert Can you file a bug? Done: https://bugs.llvm.org/show_bug.cgi?id=51913. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1067

[PATCH] D109727: [Driver] Remove unneeded *-suse-* triples

2021-10-02 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. The problem here is that triples are not just used to look up the GCC installation, but the individual components have some meaning. Changing the OS vendor from "unknown" to "suse" doesn't worry me, but dropping "-gnu" might lead us to the problems described in D10

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

2021-02-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D95754#2561849 , @jlebar wrote: > Not sure who can review this, but looking through blame it seems like maybe > @aaronpuchert? I'm by no means an expert on overloading, but this seems more like a usability question. I l

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

2021-02-15 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2310 + S.Diags.noteNumOverloadCandidatesShown(ShownOverloads); + jlebar wrote: > aaronpuchert wrote: > > Why not in the following `if`? I assume we want to show a long list not > > necess

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

2021-02-16 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2310 + S.Diags.noteNumOverloadCandidatesShown(ShownOverloads); + jlebar wrote: > aaronpuchert wrote: > > jlebar wrote: > > > aaronpuchert wrote: > > > > Why not in the following `if`? I as

[PATCH] D88220: [C++20] P1825R0: More implicit moves

2021-02-16 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D88220#2565768 , @Quuxplusone wrote: >> either way I'm a dead end — I don't consider myself authorized to "accept" >> Clang changes. >> You're still going to have to attract the attention of someone with the >> authority

[PATCH] D88220: [C++20] P1825R0: More implicit moves

2021-02-16 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D88220#2566628 , @Quuxplusone wrote: > My concern was more that I'm much less of an expert on Clang than "most of > us." Understood, but Clang tries to closely follow the C++ standard in its implementation, and I'd say t

[PATCH] D88220: [C++20] P1825R0: More implicit moves

2021-02-18 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a subscriber: aaron.ballman. aaronpuchert added a comment. In D88220#2571911 , @amccarth wrote: > Would anyone object to allowing the option but silently ignoring it, at least > for a transition period? No objection from me. > Was the

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

2021-02-20 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert accepted this revision. aaronpuchert added a comment. This revision is now accepted and ready to land. Thanks, this looks good to me. Maybe wait a few days whether @rsmith has a comment before you land it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D88220: [C++20] P1825R0: More implicit moves

2021-02-23 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D88220#2581538 , @aaron.ballman wrote: > We usually rely on the release notes to say something, but we didn't do that > here. Perhaps @nullptr.cpp could add something there? The file is `clang/docs/ReleaseNotes.rst`. >

[PATCH] D97364: [docs] Add a release note for the removing of -Wreturn-std-move-in-c++11

2021-02-24 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert accepted this revision. aaronpuchert added a comment. I assume you need someone to land this for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97364/new/ https://reviews.llvm.org/D97364 ___

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

2021-06-09 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D99696#2808592 , @Quuxplusone wrote: > @aaronpuchert what's your take on D99696 at > this point; is it good to go or still unresolved issues? Overall this looks sensible to me, and I don'

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

2021-06-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We allow branches to join where one holds a managed lock but the other doesn't, but we c

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

2021-06-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:868 ThreadSafetyHandler &Handler) const override { -if (!managed() && !asserted() && !negative() && !isUniversal()) { +if (!asserted() && !negative() && !i

[PATCH] D104649: Thread safety analysis: Rename parameters of ThreadSafetyAnalyzer::intersectAndWarn (NFC)

2021-06-21 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added subscribers: manas, steakhal, ASDenysPetrov, dkrupp, donat.nagy, Szelethus, a.sidorin, baloghadamsoftware. aaronpuchert requested review of this revision. Herald added a project: clang. Herald a

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

2021-06-21 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:2227-2228 /// \param LEK2 The error message to report if a mutex is missing from Lset2 void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1, co

[PATCH] D106713: Thread safety analysis: Warn when demoting locks on back edges

2021-08-04 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Ping @delesley. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106713/new/ https://reviews.llvm.org/D106713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

2021-06-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D104261#2832892 , @aaron.ballman wrote: > I think the CI failure (libarcher.races::lock-unrelated.c) is not related to > this patch but is a tsan thing, but you may want to double-check that just in > case. Seems that

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

2021-06-25 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:868 ThreadSafetyHandler &Handler) const override { -if (!managed() && !asserted() && !negative() && !isUniversal()) { +if (!asserted() && !negative() && !i

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

2021-06-28 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a subscriber: rupprecht. aaronpuchert added a comment. In D104261#2841356 , @delesley wrote: > since it's restricted to relockable managed locks, I'm not too worried... Not quite, it affects scoped locks with explicit unlock, which was

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

2021-06-29 Thread Aaron Puchert via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. aaronpuchert marked an inline comment as done. Closed by commit rGf664e2ec371f: Thread safety analysis: Always warn when dropping locks on back edges (authored by aaron

[PATCH] D104649: Thread safety analysis: Rename parameters of ThreadSafetyAnalyzer::intersectAndWarn (NFC)

2021-06-29 Thread Aaron Puchert 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 rGe0b90771c318: Thread safety analysis: Rename parameters of ThreadSafetyAnalyzer… (authored by aaronpuchert). Changed prior to commit: https://revi

[PATCH] D105380: DRAFT: [clang] fixes named return of variables with dependent alignment

2021-07-03 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3401 +return false; + }; + Perhaps use `std::any_of`? Maybe there is even a range wrapper in llvm. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D105380: [clang] fixes named return of variables with dependent alignment

2021-07-05 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. I'd leave it to @rsmith to decide whether this makes sense in Sema.h. I tend to follow the rule that if it's just used once I keep it local, but you might have good reasons for making it public. Comment at: clang/lib/Sema/SemaDecl.cpp:13316 + re

[PATCH] D111190: Comment parsing: Complete list of Doxygen commands

2021-11-05 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90/new/ https://reviews.llvm.org/D90 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D111262: Comment AST: Factor out function type extraction in DeclInfo::fill (NFC)

2021-11-05 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111262/new/ https://reviews.llvm.org/D111262 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D111190: Comment parsing: Complete list of Doxygen commands

2021-11-09 Thread Aaron Puchert via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. aaronpuchert marked an inline comment as done. Closed by commit rG196554d42d32: Comment parsing: Complete list of Doxygen commands (authored by aaronpuchert). Changed prior to commit: https://reviews.llvm.org/D90?vs=3

[PATCH] D112914: Misleading identifier detection

2021-11-09 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-misleading-identifier.rst:12 + +.. code-block:: c + Sphinx can't parse that (perhaps unsurprisingly), could you declare that as having no language? ``` Warning, treat

[PATCH] D111262: Comment AST: Factor out function type extraction in DeclInfo::fill (NFC)

2021-11-09 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert marked an inline comment as done. aaronpuchert added a comment. Thanks for the review! Comment at: clang/lib/AST/Comment.cpp:337 + if (Kind == TypedefKind) +Kind = FunctionKind; ParamVars = FTL.getParams(); gribozavr2 wrote: > P

[PATCH] D111264: Comment AST: Declare function pointer variables as functions

2021-11-09 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D111264#3115104 , @gribozavr2 wrote: > I feel uneasy about claiming that variable decls of function type are > FunctionKind for the purposes of comment parsing (even doing it for typedefs > is questionable). It seems li

[PATCH] D111262: Comment AST: Factor out function type extraction in DeclInfo::fill (NFC)

2021-11-09 Thread Aaron Puchert via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. aaronpuchert marked an inline comment as done. Closed by commit rG3506e42ab67e: Comment AST: Factor out function type extraction in DeclInfo::fill (NFC) (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHA

[PATCH] D111264: Comment AST: Declare function pointer variables as functions

2021-11-09 Thread Aaron Puchert 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 rG4d6382430066: Comment AST: Declare function pointer variables as functions (authored by aaronpuchert). Changed prior to commit: https://reviews.ll

[PATCH] D111266: Comment AST: Add support for variable templates

2021-11-09 Thread Aaron Puchert via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. aaronpuchert marked an inline comment as done. Closed by commit rG63ef0e17e288: Comment AST: Add support for variable templates (authored by aaronpuchert). Changed pri

[PATCH] D113425: gcc extension # NNN directive lines

2021-11-09 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/docs/LanguageExtensions.rst:4058 + +* ``1`:` Push the current source file name onto the include stack and + enter a new file. Produces ``` /home/buildbot/llvm-build-dir/clang-sphinx-docs/llvm/build/tools/cla

[PATCH] D113425: gcc extension # NNN directive lines

2021-11-09 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/docs/LanguageExtensions.rst:4058 + +* ``1`:` Push the current source file name onto the include stack and + enter a new file. urnathan wrote: > aaronpuchert wrote: > > Produces > > > > ``` > > /home/buildbot

[PATCH] D113690: Comment AST: Find out if function is variadic in DeclInfo::fill

2021-11-11 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added a reviewer: gribozavr2. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Then we don't have to look into the declaration again. Also it's only natural to collect this inform

[PATCH] D113691: Comment AST: Recognize function-like objects via return type

2021-11-11 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added a reviewer: gribozavr2. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Instead of pretending that function pointer type aliases or variables are functions, and thereby los

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

2021-11-12 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Obviously the error message was just extended by the as-written type. Could have just adapted the expected results instead of reverting... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110216/new/ https://reviews.llvm

[PATCH] D113690: Comment AST: Find out if function is variadic in DeclInfo::fill

2021-11-12 Thread Aaron Puchert 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 rG4e7df1ef7b67: Comment AST: Find out if function is variadic in DeclInfo::fill (authored by aaronpuchert). Changed prior to commit: https://reviews

[PATCH] D113691: Comment AST: Recognize function-like objects via return type (NFC)

2021-11-12 Thread Aaron Puchert 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 rG3010883fc296: Comment AST: Recognize function-like objects via return type (NFC) (authored by aaronpuchert). Changed prior to commit: https://revi

[PATCH] D113793: Comment Sema: Run checks only when appropriate (NFC)

2021-11-12 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added a reviewer: gribozavr2. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Instead of checking within the checks whether they apply, we check before calling them. This allows

[PATCH] D113794: Comment Sema: Use Name from CommandInfo for HeaderDoc diagnostics (NFC)

2021-11-12 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added a reviewer: gribozavr2. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We don't have to figure out the name, we can just fetch it from the metadata table. Also there is no

[PATCH] D113795: Comment Sema: Eliminate or factor out DeclInfo inspection (NFC)

2021-11-12 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added a reviewer: gribozavr2. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We make all predicates expect an already inspected DeclInfo, and introduce a function to run such a

[PATCH] D113793: Comment Sema: Run checks only when appropriate (NFC)

2021-11-12 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Forget to run `clang-format`, will do this when I have to update or land. Comment at: clang/lib/AST/CommentSema.cpp:628 -void Sema::checkBlockCommandDuplicate(const BlockCommandComment *Command) { - const CommandInfo *Info = Traits.getCommandInf

[PATCH] D113795: Comment Sema: Eliminate or factor out DeclInfo inspection (NFC)

2021-11-13 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/include/clang/AST/CommentSema.h:205 - /// \returns \c true if the declaration that this comment is attached to - /// is a pointer to function/method/block type or has such a type. - bool involvesFunctionType(); - - bool

[PATCH] D113837: Sema: Let InitListExpr have dependent type instead of 'void'

2021-11-13 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added a reviewer: rsmith. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. It was always just a placeholder, but not a particularly good one: the expression certainly has a value

[PATCH] D113837: Sema: Let InitListExpr have dependent type instead of 'void'

2021-11-13 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert updated this revision to Diff 387054. aaronpuchert added a comment. Herald added subscribers: kbarton, nemanjai. Herald added a project: clang-tools-extra. Adapt `clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp` and run clang-format. Repository: rG LLVM Github Monorepo C

[PATCH] D113838: Sema: Don't erroneously reject `void{}`

2021-11-13 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added a reviewer: rsmith. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is explicitly allowed via [expr.type.conv], if the initialization list is empty. Repository: rG

[PATCH] D99466: Fix PR48889: assertion failure for void() in flattened ternary expression

2021-11-13 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert updated this revision to Diff 387056. aaronpuchert marked an inline comment as done. aaronpuchert added a comment. Also fix code generation for `void{}`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99466/new/ https://reviews.llvm.org

[PATCH] D113838: Sema: Don't erroneously reject `void{}`

2021-11-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a subscriber: Tyker. aaronpuchert added a comment. CC @Tyker for the changes to `SemaCXX/attr-annotate.cpp`. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5848-5849 def err_illegal_initializer_type : Error<"illegal initializer type %0">; +def

[PATCH] D113837: Sema: Let InitListExpr have dependent type instead of 'void'

2021-11-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a subscriber: JonasToth. aaronpuchert added a comment. We could also introduce a separate (placeholder) type for initializer lists and perhaps also `ParenListExpr` if the dependent type seems misleading to you. CC @JonasToth for the changes to `clang-tidy/checkers/cppcoreguid

[PATCH] D113793: Comment Sema: Run checks only when appropriate (NFC)

2021-11-18 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D113793#3137244 , @gribozavr2 wrote: > I find the code more readable and robust when the `check*` function checks > its applicability itself. After this refactoring, it is not so clear when > each check functions applie

[PATCH] D113795: Comment Sema: Eliminate or factor out DeclInfo inspection (NFC)

2021-11-18 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D113795#3137250 , @gribozavr2 wrote: > Sorry, here I also find the old code to be more readable. Admittedly this isn't mainly about readability but about reducing boilerplate. A large number of functions start like this

[PATCH] D114562: [clang][docs] Inclusive language: remove use of sanity check in option description

2021-11-24 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/docs/ThreadSafetyAnalysis.rst:471 - + ``-Wthread-safety-attributes``: Sanity checks on attribute syntax. + + ``-Wthread-safety-attributes``: Validation checks on attribute syntax. + ``-Wthread-safety-analysis``: The cor

[PATCH] D113795: Comment Sema: Eliminate or factor out DeclInfo inspection (NFC)

2021-11-26 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments. Comment at: clang/lib/AST/CommentSema.cpp:135-137 + if (const auto *VD = dyn_cast(ThisDeclInfo->CurrentDecl)) +if (VD->getType()->isFunctionPointerType()) + return; Something might be wrong here anyway: th

[PATCH] D113837: Sema: Let InitListExpr have dependent type instead of 'void'

2021-11-29 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Ping @rsmith. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113837/new/ https://reviews.llvm.org/D113837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

2022-10-18 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In D129755#3865342 , @rupprecht wrote: > I'm seeing a fair number of breakages from this patch (not really sure how > many we truly have, I've hit ~5-10 so far in widely used libraries, but I > suspect we have far more in t

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

2022-10-19 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. Thanks for the detailed write-up, very much appreciated. In D129755#3866887 , @rupprecht wrote: > 1. (2x) Returning a MutexLock-like structure, e.g. > > MutexLock Lock() { > return MutexLock(&mu); > } > > this is doc

<    1   2   3   4   5   6   7   >