[PATCH] D129160: libclang.so: Make SONAME the same as LLVM version

aaronpuchert added a comment. > Also, this change did not really acheive it's purpose of allowing apps to use > newer versions of libclang.so without rebuilding, because a new version of > libclang.so requires a new version of libLLVM.so, which does not have a > stable ABI. Could you elaborate

[PATCH] D129160: libclang.so: Make SONAME the same as LLVM version

aaronpuchert added a comment. In D129160#3721943 , @isuruf wrote: > Sure. If an application links to `libclang.so` when the application is being > built, the application will hardcode `libclang.so.13` in it and will look for > it. > When the SONAME chan

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

aaronpuchert added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129755/new/ https://reviews.llvm.org/D129755 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

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

aaronpuchert added a comment. In D129755#3779997 , @aaron.ballman wrote: > Please be sure to add a release note for the changes! Any opinion as to what the release note might say? I'm asking since we dropped the documentation changes. Perhaps I should

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

aaronpuchert updated this revision to Diff 465782. aaronpuchert added a comment. Add trimmed-down documentation back in and a release note. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129755/new/ https://reviews.llvm.org/D129755 Files: clang/d

[PATCH] D129752: Thread safety analysis: Handle additional cast in scoped capability construction

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 rGd8fa40dfa7ad: Thread safety analysis: Handle additional cast in scoped capability construction (author

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG0041a69495f8: Thread safety analysis: Support copy-elided production of scoped capabilities… (authored by aaronpuchert). Repository: rG LLVM Githu

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

aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:1789 + auto inserted = ConstructedObjects.insert({Exp, Placeholder.first}); + assert(inserted.second && "Are we visiting the same expression again?"); + if (isa(Exp)) --

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

aaronpuchert added a comment. In D129755#3842729 , @hans wrote: > We're hitting a false positive in grpc after this: > > > ../../third_party/grpc/src/src/core/lib/gprpp/ref_counted_ptr.h:335:31: > error: calling function 'TlsSessionKeyLoggerCache' requ

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

aaronpuchert added a comment. In D129755#3843144 , @gulfem wrote: > We also started seeing `-Wthread-safety-precise` error in our Fuchsia code. > https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408

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

aaronpuchert planned changes to this revision. aaronpuchert added a comment. In D129755#3843206 , @aaronpuchert wrote: > Presumably `Cursor` is some kind of alias to `VmoCursor`, as we don't look at > base destructors yet. Since the code is not easily s

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

aaronpuchert updated this revision to Diff 466172. aaronpuchert added a comment. This revision is now accepted and ready to land. - Pass `til::LiteralPtr *Self` for automatic object destructors into handling of `require_capability` and `locks_excluded` attributes. - Add `[[maybe_unused]]` attribu

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

aaronpuchert added a comment. In D129755#3843144 , @gulfem wrote: > We also started seeing `-Wthread-safety-precise` error in our Fuchsia code. > https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408

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

aaronpuchert updated this revision to Diff 466923. aaronpuchert added a comment. Add sentence to release notes that we now look into more constructor calls and that this could lead to additional warnings being emitted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

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

aaronpuchert marked 5 inline comments as done. aaronpuchert added a comment. @hans, where you able to fix or work around the warnings? I'd like to land this again, but if you need more time it can also wait a bit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

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

aaronpuchert added a comment. @aaron.ballman, would like some feedback on the release notes. Should I additionally write something under "Potentially Breaking Changes", or is it enough to mention this under "Improvements to Clang's diagnostics"? Though I guess we could also add this later on if

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG54bfd0484615: Thread safety analysis: Support copy-elided production of scoped capabilities… (authored by aaronpuchert). Repository: rG LLVM Githu

[PATCH] D136282: [clang] [CMake] Link libclangBasic against libatomic when necessary.

aaronpuchert added inline comments. Comment at: clang/lib/Basic/CMakeLists.txt:114-117 +target_link_libraries(clangBasic + PRIVATE + ${LLVM_ATOMIC_LIB} +) Arfrever wrote: > mgorny wrote: > > Is this the right place? Grepping for `std::atomic`, I see lib/Fronten

[PATCH] D137043: [clang] add implicit include for Linux/gnu compatibility

aaronpuchert added a comment. This include-if-exists mechanism seems brittle to me. Can we not make it dependent on the triple, i.e. include the file if we're using a libc implementation that's known to provide (and require) this file? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

aaronpuchert added inline comments. Comment at: flang/lib/Frontend/FrontendActions.cpp:590 + std::optional OptLevelOrNone = + CodeGenOpt::getLevel(CGOpts.OptimizationLevel); + assert(OptLevelOrNone && "Invalid optimization level!"); Does this need `llvm::`

[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

aaronpuchert accepted this revision. aaronpuchert added a subscriber: rupprecht. aaronpuchert added a comment. This revision is now accepted and ready to land. Looks good to me, but let's wait a few days in case @aaron.ballman has anything to add. @rupprecht, in case you're still doing integrati

[PATCH] D153132: [clang analysis][NFCI] Preparatory work for D153131.

aaronpuchert added a comment. In D153132#4431627 , @courbet wrote: >> Is this actually required for the subsequent change? I don't see the >> connection. > > In the followup change, we have to check the returns after the enter and exit > CFG block are c

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

aaronpuchert added a comment. Yeah, we should get this over the line. I'm still not quite sure where to put the check. Reading @rsmith's comment again, SemaInit might perhaps be acceptable for now, except that I should add the additional tests (in case we don't have them already). I think we a

[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

aaronpuchert accepted this revision. aaronpuchert added a comment. Looks still good to me. As I wrote on D153132 , I don't think we need it anymore, but if you disagree I think I can accept it as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D153132: [clang analysis][NFCI] Preparatory work for D153131.

aaronpuchert accepted this revision. aaronpuchert added a comment. This revision is now accepted and ready to land. Got it, so it's because we want to work on a different fact set than what `BuildLockset` holds. Yeah, I think this makes sense. And after all the methods aren't too far away from

[PATCH] D157385: [clang][CFG] Cleanup functions

aaronpuchert added a comment. For me this looks good, but I'd like @NoQ to sign off on it. Comment at: clang/lib/Analysis/CFG.cpp:1874 +if (needsAutomaticDestruction(D)) DeclsNonTrivial.push_back(D); I'm wondering if you should rename this accordin

[PATCH] D152246: [clang][ThreadSafety] Analyze known function pointer values

aaronpuchert added a comment. In D152246#4484366 , @tbaeder wrote: > So, the problem with this (type of) analysis is that we don't have a perfect > view of the (global) program state, right? The CFG is per-function, and any > other function (etc.) might

[PATCH] D157385: [clang][CFG] Cleanup functions

aaronpuchert added inline comments. Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1473-1474 +// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F))); +// CHECK-NEXT:4: [B1.3].~F() (Implicit destructor) +// CHECK-NEXT:5: CleanupFunction (cleanup_F) +// CHECK-N

[PATCH] D157385: [clang][CFG] Cleanup functions

aaronpuchert added inline comments. Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480 +public: + ~F() {} +}; tbaeder wrote: > aaronpuchert wrote: > > As with the cleanup function, a definition shouldn't be necessary. > Is there a way to test whether the

[PATCH] D157385: [clang][CFG] Cleanup functions

aaronpuchert added a comment. Looks good to me, but let's wait for the CFG maintainers to approve it. Comment at: clang/lib/Analysis/ThreadSafety.cpp:2429 } + case CFGElement::TemporaryDtor: { Could you remove the added line? CHANGES SINCE L

[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

aaronpuchert added a comment. In D153131#4653345 , @aeubanks wrote: > This is finding lots of real issues in code, which is awesome, but could I > request that this be put under a separate warning flag so we can toggle off > just the new functionality a

[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

aaronpuchert added a comment. In D153131#4653412 , @courbet wrote: > I also had some push back internally on adding this to the existing flag. I'm > going to add `-Wthread-safety-reference-return`, can we start by not > temporarily including it in `-Wth

[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

aaronpuchert added a comment. In D153131#4653564 , @courbet wrote: > We have a large number of users of `-Werror -Wthread-safety-analysis` > internally. When we make the new warnings part of that flag we cannot > integrate because we're breaking all the

[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafetyCommon.cpp:320 // Substitute call arguments for references to function parameters -assert(I < Ctx->NumArgs); -return translate(Ctx->FunArgs[I], Ctx->Prev); +if (const a

[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

aaronpuchert accepted this revision. aaronpuchert added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Analysis/ThreadSafety.cpp:1776 /// \param D The callee declaration. /// \param Self If \p Exp = nullptr, the implicit this argume

[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

aaronpuchert added a comment. Side note: I hope you've seen the failing test `Analysis/scopes-cfg-output.cpp`, but since that belongs to the predecessor change I assume it's an issue there. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152504/new/ https://reviews.llvm.org/D152504 ___

[PATCH] D157385: [clang][CFG] Cleanup functions

aaronpuchert added inline comments. Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1472 +// CHECK-NEXT:2: (CXXConstructExpr, [B1.3], F) +// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F))); +// CHECK-NEXT:4: CleanupFunction (cleanup_F) Th

[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

aaronpuchert accepted this revision. aaronpuchert added a comment. Thanks, looks good! I'd like to hear from @aaron.ballman if this warrants a release note, but that can be added separately. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152504/new/ https://reviews.llvm.org/D152504 ___

[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

aaronpuchert added inline comments. Comment at: clang/test/Sema/warn-thread-safety-analysis.c:26 + // Define the mutex struct. Oh, I wouldn't mind if you were to drop this spurious added newline. But you can do this when committing, no need for a new patch se

[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

aaronpuchert added a comment. Sorry for letting this collect dust. I think it's a valuable addition, and looks pretty good, except that I think we should use the expected exit set instead of the entry set. These can be legitimately different for appropriately annotated functions, i.e. with `acq

[PATCH] D153001: [clang][ThreadSafety] Add __builtin_instance_member (WIP)

aaronpuchert added a comment. We probably parse the attributes delayed in C++ but not in C. This probably also means you can't refer to later members in the attribute, so the mutex always needs to come first, right? Not sure if such a limitation is expected for C developers. Repository: rG

[PATCH] D158775: [NFC] Initialize member pointer and avoid potential null dereference

aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:2243 const auto *CurrentFunction = dyn_cast(D); CurrentMethod = dyn_cast(D); It seems to me that `CurrentMethod` is unconditionally assigned a value here, before it is be

[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

aaronpuchert added a comment. Sorry that it took so long, I had to debug it and think a bit. This should do it: diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h index 9d28325c1ea6..13e37ac2b56b 100644 ---

[PATCH] D152246: [clang][ThreadSafety] Analyze known function pointer values

aaronpuchert added a comment. In D152246#4639495 , @tbaeder wrote: > Do you think warning on assignment of function pointers with mismatched > attributes would be a viable way forward? Yes, that sounds like the right approach. (Slightly relaxed perhaps,

[PATCH] D137043: [clang] add implicit include for Linux/gnu compatibility

aaronpuchert added a comment. In D137043#3935526 , @Origami404 wrote: > In D137043#3935129 , @aaronpuchert > wrote: > >> This include-if-exists mechanism seems brittle to me. > > Do you mean the way that we used

[PATCH] D76943: [clang analysis] Make mutex guard detection more reliable.

aaronpuchert added subscribers: aaron.ballman, aaronpuchert. aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:2144-2145 + if (auto *CE = dyn_cast(E)) +if (CE->getCastKind() == CK_NoOp || +CE->getCastKind() == CK_Construc

[PATCH] D76943: [clang analysis] Make mutex guard detection more reliable.

aaronpuchert added inline comments. Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:5659 + } + struct Convertible { Convertible(); operator ReadLockedPtr(); }; + void use_conversion() { Honestly I'd prefer the annotation to be on this operator i

[PATCH] D67112: [Sema] Add implicit cast for conversion of function references

aaronpuchert updated this revision to Diff 246862. aaronpuchert added a comment. Rebase on top of D66437 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67112/new/ https://reviews.llvm.org/D67112 Files: clang/inc

[PATCH] D67112: [Sema] Add implicit cast for conversion of function references

aaronpuchert reclaimed this revision. aaronpuchert removed a reviewer: riccibruno. aaronpuchert added a comment. The diff between both changes might still be interesting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67112/new/ https://reviews.llvm

[PATCH] D75319: Remove unused parameter from CXXRecordDecl::forallBases

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. Apparently all users of the function were fine with short-circuiting and none cared to override the default argument. Repository: rG L

[PATCH] D75319: Remove unused parameter from CXXRecordDecl::forallBases [NFC]

This revision was automatically updated to reflect the committed changes. Closed by commit rG93184a8eda27: Remove unused parameter from CXXRecordDecl::forallBases [NFC] (authored by aaronpuchert). Changed prior to commit: https://reviews.llvm.org/D75319?vs=247136&id=247442#toc Repository: rG

[PATCH] D75613: [Sema] Reword -Wrange-loop-analysis warning messages

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, Mordante, rtrieu. Herald added a project: clang. Herald added a subscriber: cfe-commits. aaronpuchert added a comment. This addresses my earlier comment D73007#1853563 . The

[PATCH] D75613: [Sema] Reword -Wrange-loop-analysis warning messages

aaronpuchert added a comment. This addresses my earlier comment D73007#1853563 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75613/new/ https://reviews.llvm.org/D75613 _

[PATCH] D75632: Comment parsing: Treat \ref as inline command

aaronpuchert created this revision. aaronpuchert added a reviewer: gribozavr2. Herald added a project: clang. Herald added a subscriber: cfe-commits. It's basically Doxygen's version of a link and can happen anywhere inside of a paragraph. Fixes a bogus warning about empty paragraphs when a parame

[PATCH] D75613: [Sema] Reword -Wrange-loop-analysis warning messages

aaronpuchert updated this revision to Diff 248325. aaronpuchert added a comment. - Change diagnostic names to reflect the changed messages. - Also change the wording of note_use_type_or_non_reference. - The warning that was warn_for_range_const_reference_copy is only emitted if the operator* call

[PATCH] D75613: [Sema] Reword -Wrange-loop-analysis warning messages

aaronpuchert updated this revision to Diff 248332. aaronpuchert added a comment. Drop SpelledAsLValue = false, which seems to be wrong. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75613/new/ https://reviews.llvm.org/D75613 Files: clang/include

[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements

aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:896 + if (S.CanPerformCopyInitialization(Entity, &AsRvalue)) +return true; +} else if (auto *FTD = dyn_cast(D)) { Overlad resolution can actually still fail if there

[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements

aaronpuchert updated this revision to Diff 226980. aaronpuchert marked an inline comment as done. aaronpuchert added a comment. Add test case where check for non-deduced parameter conversions is necessary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D69533: Thread safety analysis: Peel away NoOp implicit casts in initializers

This revision was automatically updated to reflect the committed changes. Closed by commit rGae3159e49793: Thread safety analysis: Peel away NoOp implicit casts in initializers (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements

aaronpuchert added a comment. Not answering inline because comments are getting longer. > I still think that it makes sense to do implicit-move on any control-flow > construct that "exits" the current function. Right now that's `return` and > `throw`, and they both do implicit-move (albeit inco

[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements

aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:951 Stmt *Res = new (Context) CoreturnStmt(Loc, E, PCE, IsImplicit); return Res; Another interesting question is whether this `E` should include the xvalue cast, if we did i

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

aaronpuchert added a comment. Gentle ping. I'm open to suggestions to simplify the condition, but I think templates, partial specializations and instantiations are three different things that we all need to exclude here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[PATCH] D55326: [Driver] Fix incorrect GNU triplet for PowerPC on SUSE Linux

aaronpuchert added a comment. @jrtc27, @nemanjai: Could you have a look again? I think I answered all your questions and wrote a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55326/new/ https://reviews.llvm.org/D55326

[PATCH] D65184: [Sema] Thread Safety Analysis: Make negative capability typeless.

aaronpuchert added a comment. > This is a larger effort so it will take a little bit more time on me. This is fine, but could you add a "Plan changes" action then? This will make the revision disappear from the active review queue. Repository: rL LLVM CHANGES SINCE LAST ACTION https://rev

[PATCH] D55326: [Driver] Fix incorrect GNU triplet for PowerPC on SUSE Linux

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG0010ea4224c7: [Driver] Fix incorrect GNU triplet for PowerPC on SUSE Linux (authored by aaronpuchert). Repository: rG L

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

aaronpuchert added a comment. @rsmith, could you please have a look? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68923/new/ https://reviews.llvm.org/D68923 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D67112: [Sema] Add implicit cast for conversion of function references

aaronpuchert abandoned this revision. aaronpuchert added a comment. Already fixed via D66437 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67112/new/ https://reviews.llvm.org/D67112

[PATCH] D66437: Sema: Create a no-op implicit cast for lvalue function conversions.

aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:4745 + else if (FunctionConversion) +Sequence.AddQualificationConversionStep(cv1T1, VK_LValue); Is this a qualification conversion though? The standard lists it "function

[PATCH] D75632: Comment parsing: Treat \ref as inline command

This revision was automatically updated to reflect the committed changes. Closed by commit rGf23df1b2a323: Comment parsing: Treat \ref as inline command (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75632/new/ https://rev

[PATCH] D71966: [Wdocumentation][RFC] Improve identifier's of \param

aaronpuchert added a comment. Thanks for working on this, I'd like to see this being fixed. Comment at: clang/include/clang-c/Documentation.h:383 CINDEX_LINKAGE -CXString clang_ParamCommandComment_getParamName(CXComment Comment); gribozavr2 wrote: > Mordante

[PATCH] D75613: [Sema] Reword -Wrange-loop-analysis warning messages

This revision was automatically updated to reflect the committed changes. Closed by commit rG33bb32bbc674: [Sema] Reword -Wrange-loop-analysis warning messages (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75613/new/ http

[PATCH] D72635: Allow arbitrary capability name in Thread Safety Analysis

aaronpuchert added a comment. @aaron.ballman, I've just noted that one of the `-Wthread-safety-attributes` warnings says > //A// attribute can only be applied in a context annotated with > ‘capability(“mutex”)’ a

[PATCH] D72635: Allow arbitrary capability name in Thread Safety Analysis

aaronpuchert added a comment. In D72635#1911671 , @aaron.ballman wrote: > However, do we want to diagnose when the capability strings are different? Hmm, interesting question. The way I think about ordering capabilities is this: assuming I have more th

[PATCH] D72635: Allow arbitrary capability name in Thread Safety Analysis

aaronpuchert added a comment. In D72635#1912501 , @aaron.ballman wrote: > In D72635#1911844 , @aaronpuchert > wrote: > > > In D72635#1911671 , @aaron.ballman > > wrote: >

[PATCH] D76038: PR45000: Use Sema::SetParamDefaultArgument in TransformLambdaExpr

aaronpuchert created this revision. aaronpuchert added a reviewer: rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. We have to properly initialize from the given initialization expression to make types match. Repository: rG LLVM Github Monorepo https://reviews.ll

[PATCH] D76038: PR45000: Use Sema::SetParamDefaultArgument in TransformLambdaExpr

aaronpuchert planned changes to this revision. aaronpuchert added a comment. In D76038#1918571 , @aaronpuchert wrote: > @rsmith, should I put a testcase under `test/SemaTemplate/`, perhaps as > `instantiate-lambda.cpp`? I'll probably put it into `test/

[PATCH] D76038: PR45000: Use Sema::SetParamDefaultArgument in TransformLambdaExpr

aaronpuchert added a comment. @rsmith, should I put a testcase under `test/SemaTemplate/`, perhaps as `instantiate-lambda.cpp`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76038/new/ https://reviews.llvm.org/D76038 ___

[PATCH] D76038: PR45000: Use Sema::SetParamDefaultArgument in TransformLambdaExpr

aaronpuchert added a comment. Seems the same issue in `Sema::SubstParmVarDecl` was fixed in rGdc40b618cf397df7369406b3f61e91ccb57fb9f6 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

This revision was automatically updated to reflect the committed changes. Closed by commit rG27684ae66d55: Don't warn about missing declarations for partial template specializations (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

aaronpuchert added a comment. Thanks for the reviews! Do you think it makes sense to bring this to Clang 10? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68923/new/ https://reviews.llvm.org/D68923 ___

[PATCH] D67113: ICK_Function_Conversion is a third kind conversion

aaronpuchert added a comment. @rsmith Could you have a look at this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67113/new/ https://reviews.llvm.org/D67113 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

aaronpuchert added a comment. In D68923#1853303 , @aaron.ballman wrote: > In D68923#1853302 , @aaronpuchert > wrote: > > > Thanks for the reviews! Do you think it makes sense to bring this to Clang > > 10? > > >

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. I've been thinking about the warning messages, which seem to a bit inaccurate. Sorry for piggy-backing this onto the change, I hope you don't mind. Comment at: clang/lib/Sema/SemaStmt.cpp:2758-2759 // the correct time to bind a const referenc

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

aaronpuchert added a comment. In D68923#1853527 , @aaronpuchert wrote: > In D68923#1853303 , @aaron.ballman > wrote: > > > I think it's a simple enough fix that it may be worth it, but it isn't > > fixing a regre

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

aaronpuchert added a subscriber: hans. aaronpuchert added a comment. @hans, could you cherry-pick this on the version 10 branch? As I wrote in D68923#1857046 , this is a regression from Clang 8. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

aaronpuchert added a comment. Ping @aaron.ballman. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66919/new/ https://reviews.llvm.org/D66919 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

aaronpuchert added inline comments. Comment at: clang/test/Sema/warn-strict-prototypes.c:11 +// function definition with 0 params, no prototype. +void foo1() {} // expected-warning {{this old-style function definition is not preceded by a prototype}} +// function definition with

[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

aaronpuchert added inline comments. Comment at: clang/test/Sema/warn-strict-prototypes.c:11 +// function definition with 0 params, no prototype. +void foo1() {} // expected-warning {{this old-style function definition is not preceded by a prototype}} +// function definition with

[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

aaronpuchert marked 5 inline comments as done. aaronpuchert added a comment. Thanks! Comment at: clang/test/Sema/warn-strict-prototypes.c:11 +// function definition with 0 params, no prototype. +void foo1() {} // expected-warning {{this old-style function definition is not pre

[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

This revision was automatically updated to reflect the committed changes. aaronpuchert marked an inline comment as done. Closed by commit rG2f26bc554270: Warn about zero-parameter K&R definitions in -Wstrict-prototypes (authored by aaronpuchert). Changed prior to commit: https://reviews.llvm.or

[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

aaronpuchert added a comment. Just FYI, I had to fix some tests after this in rG705306526b5ca7eed2fa28ebf832873cbb5085ec . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66919/new

[PATCH] D72635: Add "context" capability to Thread Safety Analysis

aaronpuchert added a comment. Hmm, I have been wondering about this as well. The way I see it, all of these things are what we call //capabilities//, and we treat them all the same. The names are just meant to make warning messages more readable, because what the analysis considers a capability

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. As I wrote on the bug , I think we should only suppress one of the warnings in templates (and maybe always): :18:22: warning: loop variable '_' is always a copy because the range of type 'const Rng' does not return a r

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. In D73007#1829709 , @xbolva00 wrote: > Yes, but minimal fix is better for release branch, so @hans should merge it. I think `-Wall` shouldn't warn about reference types in a range-based for loop (unless it's the wrong type

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. I also still think that `warn_for_range_copy` //should be// emitted even in templates. These aren't false positives in my opinion, especially since we're now pretty conservative about that warning. Comment at: clang/lib/Sema/SemaStmt.cpp:2703-27

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. I think we can actually view this in more general terms, unrelated to range-based for loops. // X is non-trivially copyable. struct X { X(const X&); }; struct Y { Y(const X&); }; X retVal(); const X& retRef(); // In function scope ... const X &x1

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. In D73007#1837271 , @aaronpuchert wrote: > one could also argue that this is a bit harder to follow in a range-based for > loop. This seems to be the argumentation of https://bugs.llvm.org/show_bug.cgi?id=32823#c0, so I g

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. Here is a proposal: we add two children to `-Wrange-loop-analysis`. - `-Wrange-loop-construct` warns about possibly unintended constructor calls. This might be in `-Wall`. It contains - `warn_for_range_copy`: loop variable A of type B creates a copy from type C

[PATCH] D73434: [Sema] Remove a -Wrange warning from -Wall

aaronpuchert added a comment. Looks right to me, but someone else should approve. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73434/new/ https://reviews.llvm.org/D73434 ___ cfe-commits mailing list c

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

aaronpuchert added a comment. Another ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68923/new/ https://reviews.llvm.org/D68923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

<    1   2   3   4   5   6   7   >