[PATCH] D138914: Make evaluation of nested requirement consistent with requires expr.

rtrieu added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2339 +Req->getConstraintExpr()->getSourceRange(), Satisfaction)) + TransConstraint = Result[0]; +assert(!Trap.hasErrorOccurred() && "Substitution failures must be handled "

[PATCH] D136564: [clang] Instantiate NTTPs and template default arguments with sugar

rtrieu added a comment. I noticed some build failures after your commit. I'm trying to get a standalone reproducer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136564/new/ https://reviews.llvm.org/D136564

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rtrieu added inline comments. Comment at: clang/include/clang/AST/Type.h:3947 + Info) { +FunctionTypeBits.FastTypeQuals = 0; + } rmaz wrote: > aaron.ballman wrote: > > It seems a bit odd to me that we only want to initialize one member

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

rtrieu added a comment. In D133586#3781468 , @rmaz wrote: > In D133586#3781381 , @rtrieu wrote: > >> Do you have a test case for this? > > Was struggling to think of a good one. How about a test that repeatedly

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

rtrieu added a comment. Do you have a test case for this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 ___ cfe-commits mailing list

[PATCH] D130510: Missing tautological compare warnings due to unary operators

rtrieu added a comment. In D130510#3728719 , @aaron.ballman wrote: > In D130510#3727096 , @rtrieu wrote: > >> This patch has been moving back and forth between >> `IsIntegerLiteralConstantExpr` and

[PATCH] D130510: Missing tautological compare warnings due to unary operators

rtrieu added a comment. This patch has been moving back and forth between `IsIntegerLiteralConstantExpr` and `getIntegerLiteralSubexpressionValue`. The first function is preexisting and the second one is a new function. The final patch seems to settle on using just

[PATCH] D131532: [Sema] Avoid isNullPointerConstant invocation

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. I think this is a reasonable step for improving compile times. I put some suggestions for more descriptive names below (he said, after suggesting those names in the first place). The

[PATCH] D130510: Missing tautological compare warnings due to unary operators

rtrieu added a comment. void foo(long x) { if ((x & 1) == 1L) return; // bad always false warning here static_assert(sizeof(int) < sizeof(long), "long is bigger than int"); static_assert((long(15) & 1) == 1L, "proof that condition can be true"); } I found this false positive

[PATCH] D130510: Missing tautological compare warnings due to unary operators

rtrieu added a comment. Can you add my earlier test case or something like it to SemaCXX/warn-bitwise-compare.cpp ? template void foo(int x) { bool b1 = (x & sizeof(T)) == 8; bool b2 = (x & I) == 8; bool b3 = (x & 4) == 8; // only warn here } void run(int x) {

[PATCH] D130510: Missing tautological compare warnings due to unary operators

rtrieu added a comment. In D130510#3693494 , @aaron.ballman wrote: > In D130510#3692654 , @rtrieu wrote: > >> Because of this, warnings should treat dependent expressions as non-constant >> even when they can

[PATCH] D130510: Missing tautological compare warnings due to unary operators

rtrieu reopened this revision. rtrieu added a comment. This revision is now accepted and ready to land. This warning is now flagging some code which I believe is a false positive. In particular, when template arguments are involved, their values can be calculated during template instantiation,

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

rtrieu added inline comments. Comment at: clang/lib/AST/QualTypeNames.cpp:455 + if (const auto *UT = QT->getAs()) { +return getFullyQualifiedType(UT->getUnderlyingType(), Ctx, + WithGlobalNsPrefix); Moving this down here

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

rtrieu added a comment. This seems to be acting weird in template instantations. Here's an example where the lambda only errors inside a template. template int foo(int x = 0) { auto lambda = [x = x+1]() -> decltype(x) { return x; }; return -1; } // no

[PATCH] D119496: [Clang][OpaquePtr] Remove calls to deprecated Address constructor

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd5c314cdf43a: [Clang][OpaquePtr] Remove deprecated Address constructor calls (authored by rtrieu). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D119496: [Clang][OpaquePtr] Remove calls to deprecated Address constructor

rtrieu created this revision. rtrieu added reviewers: cfe-commits, aeubanks. rtrieu requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. Herald added a project: clang. Explicitly specify element type when constructing Address in CGExpr.cpp.

[PATCH] D117376: Remove reference type when checking const structs

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbe2147db054e: Remove reference type when checking const structs (authored by rtrieu). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D117376: Remove reference type when checking const structs

rtrieu created this revision. rtrieu added reviewers: rsmith, cfe-commits. rtrieu requested review of this revision. Herald added a project: clang. ConstStructBuilder::Finalize in CGExprConstant.ccp assumes that the passed in QualType is a RecordType. In some instances, the type is a reference

[PATCH] D114382: [clang] Fix wrong -Wunused-local-typedef warning within a template function

rtrieu added inline comments. Comment at: clang/test/Modules/odr_hash.cpp:4288 S s; +// expected-error@first.h:* {{'ParameterTest::S::Foo' has different definitions in different modules; definition in module 'FirstModule' first difference is 1st parameter with name ''}}

[PATCH] D108794: Fully qualify template template parameters when printing

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. In D108794#2978134 , @dblaikie wrote: > Ping > > In D108794#2976063 , @rtrieu wrote: > >> It looks like a

[PATCH] D108794: Fully qualify template template parameters when printing

rtrieu added a comment. It looks like a strict improvement on printing and most callers using the default args won't need to be updated. There's one more function call that should be updated: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/DumpAST.cpp#L298 Fixing that

[PATCH] D101387: remove single quotes around sugguestion diagnostic

rtrieu added a comment. In D101387#2720500 , @MaskRay wrote: > @rtrieu Do we have a way appending arbitrary messages to a diagnostic > template? Not an arbitrary number. The diagnostic format string is indexed, so it diagnostic string needs to know

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/Sema/SemaChecking.cpp:10166 + /// The number of bits active in the int. Note that this includes exactly one + /// sign bit if !NoNegative.

[PATCH] D85574: [Sema] Fix missing warning on initializer lists on field initializers with overloaded operators

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85574/new/ https://reviews.llvm.org/D85574 ___

[PATCH] D85574: [Sema] Fix missing warning on initializer lists on field initializers with overloaded operators

rtrieu added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3584-3585 if (!isa(Base->IgnoreParenImpCasts())) return; Does the warning work if it was changed to be "Visit(Base);" before the return here instead of the change

[PATCH] D85287: Extend -Wtautological-bitwise-compare "bitwise or with non-zero value" warnings

rtrieu added a comment. In D85287#2199490 , @sberg wrote: > In D85287#2199463 , @rtrieu wrote: > >> Also, have you tried running warning over a codebase? > > As I wrote: "At least building LibreOffice with this

[PATCH] D85287: Extend -Wtautological-bitwise-compare "bitwise or with non-zero value" warnings

rtrieu added a subscriber: AndersRonnholm. rtrieu added a comment. I looked back on the commits and while I did commit, I did it on the behalf of Anders Rönnholm, who did not have commit access at the time. I haven't seen activity from Anders in a while, but added to subscribers just in case.

[PATCH] D85256: Add -Wtautological-value-range-compare warning.

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85256/new/ https://reviews.llvm.org/D85256 ___

[PATCH] D79548: Fix false positive with warning -Wnon-c-typedef-for-linkage

This revision was automatically updated to reflect the committed changes. Closed by commit rG4ae537c2220f: Fix false positive with -Wnon-c-typedef-for-linkage (authored by rtrieu). Changed prior to commit: https://reviews.llvm.org/D79548?vs=262536=262808#toc Repository: rG LLVM Github

[PATCH] D79548: Fix false positive with warning -Wnon-c-typedef-for-linkage

rtrieu created this revision. rtrieu added a reviewer: rsmith. Herald added a project: clang. Fix false positives for -Wnon-c-typedef-for-linkage Implicit methods for structs can confuse the warning, so exclude checking the Decl's that are implicit. Implicit Decl's for lambdas still need to be

[PATCH] D73762: New warning for for-loops where the iteration does not match the loop condition

rtrieu created this revision. This is a warning for when the increment/decrement in a for loop does not match the condition in the loop. If the condition has a relational operator, one operand can be deduced to be the larger value and the other operand the smaller value when the loop is run.

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

rtrieu added a comment. I'm in favor of splitting the warning into subgroups, then deciding which ones should be in -Wall. We've done this with other warnings such as the conversion warnings and tautological compare warnings, with only a subset of them in -Wall. Repository: rG LLVM Github

[PATCH] D72552: [Concepts] Constraint Satisfaction Caching

rtrieu added inline comments. Comment at: clang/lib/AST/ASTConcept.cpp:17 #include "clang/AST/ASTContext.h" +#include "clang/Sema/SemaConcept.h" using namespace clang; This causes a circular dependency between AST and Sema. It looks like you are including

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

rtrieu added a comment. Heads up in case it affects you refactoring work: While looking through this code, I found a bug I previously made. I fixed it with a small change, but that lies in the middle of your refactoring during FieldDecl handling. The change is here:

[PATCH] D72551: Warn when a string literal is used in a range-based for-loop

rtrieu created this revision. String literals used in ranged-based for-loops will actually process the terminating null character as part of the loop, which may be unexpected. // This runs three times, once for c = 'h', then c = 'i', and finally as c = '\0' for (char c : "hi") This patch

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

rtrieu added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:11007 + // Issue any pending ODR-failure diagnostics. + for (auto : RecordOdrMergeFailures) { bruno wrote: > rtrieu wrote: > > Is this just a copy of the other loop? That's a

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

rtrieu added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:11007 + // Issue any pending ODR-failure diagnostics. + for (auto : RecordOdrMergeFailures) { Is this just a copy of the other loop? That's a lot of code duplication.

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

rtrieu added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:2812-2813 - // TODO: Determine a maximum size that a POD type can be before a diagnostic - // should be emitted. Also, only ignore POD types with trivial copy - // constructors. - if

[PATCH] D71503: New warning on for-loops that never run because condition is false on the first iteration

rtrieu updated this revision to Diff 234180. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71503/new/ https://reviews.llvm.org/D71503 Files: clang/include/clang/AST/Expr.h clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/Sema/SemaStmt.cpp

[PATCH] D71503: New warning on for-loops that never run because condition is false on the first iteration

rtrieu added a comment. In D71503#1784697 , @lebedev.ri wrote: > Thank you for working on this! > This seems to be missing tests. My bad, still getting used to git. Updated with a test now. CHANGES SINCE LAST ACTION

[PATCH] D71503: New warning on for-loops that never run because condition is false on the first iteration

rtrieu created this revision. This is a proposed warning to be added to -Wfor-loop-analysis, which is part of -Wall. This warning will catch instances where the condition will be false on the first iteration and the loop body will never be run. The warning will be emitted when: 1. The

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

rtrieu added a comment. In D69292#1747062 , @mclow.lists wrote: > Sorry I'm late to the party; I've been traveling for 3+ weeks. > I would like to be reassured that the following code will not warn: > > ` >long foo = ...; // some calculation >

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

This revision was automatically updated to reflect the committed changes. Closed by commit rG9740f9f0b6e5: Add -Wtautological-compare to -Wall (authored by rtrieu). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D69292?vs=228361=228970#toc Repository: rG

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

rtrieu updated this revision to Diff 228361. rtrieu added a comment. Add -Wall tests to check that certain warning groups are active with it and a test to check all warning groups that are turned on by -Wall. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69292/new/

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

rtrieu added a comment. In D69292#1718415 , @thakis wrote: > Abstractly this lgtm. Do you have any data on true / false positive rates? Looking at the various warnings in -Wtautological-compare, only -Wtautological-overlap-compare and

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

rtrieu added a comment. In D66046#1717229 , @thakis wrote: > Mr Trieu, what do you think about adding some or all of the > Wtautological-compare warnings to Wall I thought -Wtautological-compare was already part of -Wall, so I'm all for adding it. I

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

rtrieu created this revision. Per https://reviews.llvm.org/D66046, patch to move -Wtautological-compare to -Wmost, which also adds it to -Wall. Some warnings in -Wtautological-compare and its diagnostic sub-groups are DefaultIgnore, so making them visible in -Wall will make them more

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

This revision was automatically updated to reflect the committed changes. Closed by commit rG637af4cc3780: Add -Wbitwise-conditional-parentheses to warn on mixing | and with ?: (authored by rtrieu). Herald added a project: clang. Changed prior to commit:

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

This revision was automatically updated to reflect the committed changes. Closed by commit rG8b0d14a8f0cc: New tautological warning for bitwise-or with non-zero constant always true. (authored by rtrieu). Herald added a project: clang. Changed prior to commit:

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

rtrieu added a comment. In D66046#1696785 , @xbolva00 wrote: > Could this patch solve https://bugs.llvm.org/show_bug.cgi?id=43573? No, but I left some notes on the bug on why negative values are hard and where to fix it. CHANGES SINCE LAST ACTION

[PATCH] D66045: Improve detection of same value in comparisons

This revision was automatically updated to reflect the committed changes. Closed by commit rL372453: Merge and improve code that detects same value in comparisons. (authored by rtrieu, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

This revision was automatically updated to reflect the committed changes. Closed by commit rL372448: Improve -Wtautological-overlap-compare (authored by rtrieu, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

rtrieu updated this revision to Diff 219213. rtrieu added a comment. Add more test cases and split new warnings into a separate warning group, but still under -Wparentheses CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 Files:

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

rtrieu marked an inline comment as done. rtrieu added a comment. In D66044#1626008 , @NoQ wrote: > Looks great, thanks! I'd appreciate direct CFG tests for the part that > changes the CFG (cf. `test/Analysis/cfg.cpp`), but i don't insist. Let's make >

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

rtrieu updated this revision to Diff 215021. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66044/new/ https://reviews.llvm.org/D66044 Files: lib/Analysis/CFG.cpp lib/Analysis/ReachableCode.cpp test/Analysis/cfg.cpp test/Sema/warn-overlap.c test/Sema/warn-unreachable.c

[PATCH] D66045: Improve detection of same value in comparisons

rtrieu marked an inline comment as done. rtrieu added inline comments. Comment at: test/SemaCXX/self-comparison.cpp:87 +}; +} // namespace member_tests jfb wrote: > rtrieu wrote: > > rtrieu wrote: > > > jfb wrote: > > > > The test only has `==`. Do other

[PATCH] D66045: Improve detection of same value in comparisons

rtrieu updated this revision to Diff 215002. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66045/new/ https://reviews.llvm.org/D66045 Files: include/clang/AST/Expr.h lib/AST/Expr.cpp lib/Analysis/CFG.cpp lib/Sema/SemaExpr.cpp test/Analysis/array-struct-region.cpp

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

rtrieu marked an inline comment as done. rtrieu added inline comments. Comment at: test/Sema/parentheses.c:148 + + (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}} + (void)(x & b ? 1 : 2); //

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

rtrieu marked an inline comment as done. rtrieu added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator jfb wrote: >

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

rtrieu updated this revision to Diff 214753. rtrieu added a comment. Update comments to explain why bitwise-xor is treated as a logical operator. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 Files: lib/Sema/SemaExpr.cpp

[PATCH] D66045: Improve detection of same value in comparisons

rtrieu marked 3 inline comments as done. rtrieu added inline comments. Comment at: test/Analysis/array-struct-region.cpp:31 + bool check() const { return this == this + 0; } + bool operator !() const { return this != this + 0; } NoQ wrote: > rtrieu wrote: >

[PATCH] D66045: Improve detection of same value in comparisons

rtrieu updated this revision to Diff 214752. rtrieu added a comment. Check array accesses. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66045/new/ https://reviews.llvm.org/D66045 Files: include/clang/AST/Expr.h lib/AST/Expr.cpp lib/Analysis/CFG.cpp lib/Sema/SemaExpr.cpp

[PATCH] D66045: Improve detection of same value in comparisons

rtrieu marked 2 inline comments as done. rtrieu added inline comments. Comment at: lib/AST/Expr.cpp:3931-3932 +case DeclRefExprClass: { + // DeclRefExpr without an ImplicitCastExpr can happen for integral + // template parameters. + const auto *DRE1 =

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

rtrieu marked 2 inline comments as done. rtrieu added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161 + "bitwise or with non-zero value always evaluates to true">, + InGroup, DefaultIgnore; def warn_tautological_overlap_comparison : Warning<

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

rtrieu updated this revision to Diff 214733. rtrieu added a comment. Create new function isIntOrEnumConstantZero to use instead of EvaluateAsInt CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66046/new/ https://reviews.llvm.org/D66046 Files: include/clang/Analysis/CFG.h

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

rtrieu marked an inline comment as done. rtrieu added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator jfb wrote: >

[PATCH] D66045: Improve detection of same value in comparisons

rtrieu marked 3 inline comments as done. rtrieu added a comment. In D66045#1624389 , @jfb wrote: > Does this work for unions as well? This will work for union accesses via the same member name, but not different member names. union U { int x; int y;

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

rtrieu marked an inline comment as done. rtrieu added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161 + "bitwise or with non-zero value always evaluates to true">, + InGroup, DefaultIgnore; def warn_tautological_overlap_comparison : Warning<

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

rtrieu marked an inline comment as done. rtrieu added inline comments. Comment at: test/Sema/warn-unreachable.c:437 + if (~ 1) return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] unaryOpNoFixit(); // expected-warning {{code will never be executed}} jfb

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

rtrieu marked 2 inline comments as done. rtrieu added inline comments. Comment at: test/Sema/parentheses.c:148 + + (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}} + (void)(x & b ? 1 : 2); //

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63789/new/ https://reviews.llvm.org/D63789

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

rtrieu added inline comments. Comment at: clang/lib/AST/ODRHash.cpp:73 AddBoolean(S.isUnarySelector()); unsigned NumArgs = S.getNumArgs(); for (unsigned i = 0; i < NumArgs; ++i) { vsapsai wrote: > rtrieu wrote: > > There's actually a second bug

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

rtrieu added inline comments. Comment at: clang/lib/AST/ODRHash.cpp:73 AddBoolean(S.isUnarySelector()); unsigned NumArgs = S.getNumArgs(); for (unsigned i = 0; i < NumArgs; ++i) { There's actually a second bug here as well. When processing an

[PATCH] D58122: Restore Check for Unreachable Exit Block in -Winfinite-recursion

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58122/new/ https://reviews.llvm.org/D58122 ___ cfe-commits

[PATCH] D52218: Warn on self-initializations

rtrieu created this revision. Improve some diagnostics around self-initialization. The uninitialized checker does not warn on self-initialization, but does warn later if the variable is used. GCC also has a -Winit-self which will enable a -Wuninitialized warning for self-initialized.

[PATCH] D48524: [ODRHash] Rip out the registration of Type* in TypeMap

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. LGTM Thank you for reducing this test case for ODR hashing. https://reviews.llvm.org/D48524 ___ cfe-commits mailing list

[PATCH] D48524: [ODRHash] Do not put elaborated types in the TypeMap

rtrieu added a comment. Hey Vassil, I have been giving this a little more thought and I have a solution I'd like you to try out. If the type pointers aren't unique enough, then the indexing scheme will not work for hashing. Can you try this idea in your build environment and see if works

[PATCH] D48524: [ODRHash] Do not put elaborated types in the TypeMap

rtrieu added a comment. Running this, I see that it causes a regression in one of the other ODR Hash tests. Specifically, the diagnostics on line 1150 & 1151 of test/Modules/odr_hash.cpp are not seen. Was that an expected side-effect of this patch? Repository: rC Clang

[PATCH] D47340: [ClangDiagnostics] Silence warning about fallthrough after PrintFatalError

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D47340 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47340: [ClangDiagnostics] Silence warning about fallthrough after PrintFatalError

rtrieu added a comment. Instead of adding a break, you should add a noreturn attribute to the PrintFatalError function. LLVM has the macro LLVM_ATTRIBUTE_NORETURN to do this. You can confirm that PrintFatalError is a noreturn function by seeing that it unconditionally calls

[PATCH] D43696: Reduce hash collisions for reference and pointer types

rtrieu added a comment. r328404 has been committed to support references and pointers. Repository: rC Clang https://reviews.llvm.org/D43696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43737: Improve -Winfinite-recursion

rtrieu accepted this revision. rtrieu added a comment. This revision is now accepted and ready to land. Looks good. Ready to commit. https://reviews.llvm.org/D43737 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.

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 rL327598: Refactoring code around move/copy initialization. NFC. (authored by rtrieu, committed by ). Herald added a

[PATCH] D43737: Improve -Winfinite-recursion

rtrieu added a comment. Two more changes, then everything is good to commit. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:218-220 +// Found a path to the exit node without a recursive call. +if (ExitID == Block->getBlockID()) + return false;

[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.

rtrieu added a comment. Everything looks good and ready for commit. Repository: rC Clang https://reviews.llvm.org/D43898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43737: Improve -Winfinite-recursion

rtrieu added a comment. > I believe you were around this code last, so can you remember why it was > there? Yes, that's an early exit to speed up the check. You can remove that check and add a test case for it. This was a little confusing for me, so let's refactor it a little. These

[PATCH] D40731: Integrate CHash into CLang

rtrieu added inline comments. Herald added a subscriber: JDevlieghere. Comment at: include/clang/AST/CHashVisitor.h:72-79 + template + void addData(const llvm::iterator_range ) { +addData(std::distance(x.begin(), x.end())); + } + template + void addData(const

[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.

rtrieu added inline comments. Comment at: lib/Sema/SemaStmt.cpp:2953 + ExprResult ) +{ + ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(), Opening brace should follow the closing paren on previous line.

[PATCH] D43696: Reduce hash collisions for reference and pointer types

rtrieu added inline comments. Comment at: lib/AST/ODRHash.cpp:581 + void VisitType(const Type *T) { +ID.AddInteger(T->getTypeClass()); + } v.g.vassilev wrote: > rtrieu wrote: > > rsmith wrote: > > > This looks redundant, the above `Visit(const Type*)`

[PATCH] D43737: Improve -Winfinite-recursion

rtrieu added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:255-257 // If the exit block is unreachable, skip processing the function. if (cfg->getExit().pred_empty()) return; This is likely what is causing my previous code example to

[PATCH] D43737: Improve -Winfinite-recursion

rtrieu added a comment. Can you explain the new algorithm for checking recursive calls? From the test, this code looks like what you are testing for, but it doesn't trigger the warning. void f() { while(true) { f(); } } https://reviews.llvm.org/D43737

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

rtrieu added a comment. > I have one very minor nit that I don't know how to fix: > > warn-return-std-move.cpp:220:12: warning: local variable 'd' will be copied > despite being returned by name [-Wreturn-std-move] > return (d); // e17 > ^ >

[PATCH] D43696: Reduce hash collisions for reference and pointer types

rtrieu added inline comments. Comment at: lib/AST/ODRHash.cpp:581 + void VisitType(const Type *T) { +ID.AddInteger(T->getTypeClass()); + } rsmith wrote: > This looks redundant, the above `Visit(const Type*)` function seems to > already do this. That's

[PATCH] D34810: [Sema] -Wcomma should not warn for expressions that return void

rtrieu added a comment. Reid is correct, the whitelisted expressions was greatly reduced during code review so only casts to void would disable the warning. While the last review did not have the description updated to reflect this, the committed code does have an accurate description. What

[PATCH] D21675: New ODR checker for modules

rtrieu added a comment. This patch will be landing in small chunks to hopefully avoid the large reverts. Repository: rL LLVM https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D21675: New ODR checker for modules

This revision was automatically updated to reflect the committed changes. Closed by commit rL293585: Add better ODR checking for modules. (authored by rtrieu). Changed prior to commit: https://reviews.llvm.org/D21675?vs=86142=86376#toc Repository: rL LLVM https://reviews.llvm.org/D21675

[PATCH] D21675: New ODR checker for modules

rtrieu updated this revision to Diff 86142. rtrieu added a comment. Changes made to the ODR hash algorithm: Separated Decl and Type pointers into their own DenseMap's. Removed the queue of pointers to process at the end. Instead, process pointers at first use. Save Boolean values and add them

[PATCH] D21675: New ODR checker for modules

rtrieu added a comment. In https://reviews.llvm.org/D21675#654659, @teemperor wrote: > I feel like we have a really similar use case in the > Analysis/CloneDetection{.h/.cpp} with the hashing of statements. I tried > substituting the call to the statement hashing with a call to the >

[PATCH] D21675: New ODR checker for modules

rtrieu added a comment. After changing the ODRHash class a bit, the new performance numbers are 3% in debug and 1-1.5% in release builds. https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D21675: New ODR checker for modules

rtrieu marked an inline comment as done. rtrieu added a comment. From testing, there is no difference when compiling with pre-compiled headers. However, when building the headers, there is a 3-4% impact on compile time. https://reviews.llvm.org/D21675

  1   2   >