[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check
JonasToth added a comment. IMHO the check looks good, but I do have concerns about correctness of the transformation in specific cases, especially overloaded operators. If the conditions are inverted, it might be the case that the inverted operator is not overloaded, resulting in compilation errors. I think that should be guarded against. And then there is the more subtle issue of different semantics of the operators. Hypothetical Matrix Algebra Situation: // A, B are matrices of booleans with identical dimensions // A && B will do '&&' on each matrix element // The matrices overload 'operator bool()' for implicit conversion to 'bool', // true := any element != false // false := all false if (A && B) { // !C inverts each boolean in C, same '&&' application if (B && !C) { padLines(); } } Transformed to if (!A ) continue; if ( !B) continue; if (!B ) continue; if ( C) continue; padLines(); is highly likely not a correct and equivalent transformation. My point is not so much about the concrete example, but more general. C++ allows to implement operators with different semantics and classes must not behave like 'int' for the operators. Such overloads are not necessarily incorrect. E.g. `boost::sml` uses the operator overloading to define state machines, which does not follow anything close to the semantics we need for this check. Expression-template libraries (especially linear algebra and so on) might either break or change meaning as well. In my personal opinion the check should at least have an option to disable transformations for classes with overloaded operators and verify that the transformation would still compile if done anyway. I think even better would be a "concept check" to at least verify that the type in question models a boolean with its operators. But I am not sure how far such a check should go and I am aware that it would not be perfect anyway. Comment at: clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp:329 +assert(BSize >= CheckAlias.size() + OptName.size()); +memcpy(Buff, CheckAlias.data(), CheckAlias.size()); +memcpy(Buff + CheckAlias.size(), OptName.data(), OptName.size()); i would really prefer to just use strings here. `std::string Buff = CheckAlias.str() + OptName.str();` is much easier to understand and equivalent (?) this function is called only once in the constructor as well, so speed and allocations are not valid in my opinion. Comment at: clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst:63 +void Process(bool A, bool B) { + if (A && B) { +// Long processing. njames93 wrote: > JonasToth wrote: > > njames93 wrote: > > > JonasToth wrote: > > > > if this option is false, the transformation would be `if(!(A && B))`, > > > > right? > > > > > > > > should demorgan rules be applied or at least be mentioned here? I think > > > > transforming to `if (!A || !B)` is at least a viable option for enough > > > > users. > > > Once this is in, I plan to merge some common code with the > > > simplify-boolean-expr logic for things like demorgan processing. Right > > > now the transformation happens, the simplify boolean suggests a demorgan > > > transformation of you run the output through clang tidy. > > a short reference to the `readability-simplify-boolean-expr` check in the > > user facing docs would be great. > > i personally find it ok if the users "pipe" multiple checks together to > > reach a final transformation. > > > > would this check then use the same settings as the > > `readability-simplify-boolean-expr` check? (that of course off topic and > > does not relate to this patch :) ) > I'm not sure it's really necessary to mention that the fix would likely need > another fix, besides that comment would just be removed in the follow up. ok, thats good with me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130181/new/ https://reviews.llvm.org/D130181 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO
azat updated this revision to Diff 457850. azat added a comment. Adjust the test (to fix build on windows) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133092/new/ https://reviews.llvm.org/D133092 Files: clang/lib/Driver/ToolChains/CommonArgs.cpp clang/test/Driver/debug-options.c Index: clang/test/Driver/debug-options.c === --- clang/test/Driver/debug-options.c +++ clang/test/Driver/debug-options.c @@ -246,7 +246,11 @@ // RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=NOPUB %s // RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s // -// RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -flto -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=LDGARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -flto=thin -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=LDGARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -fuse-ld=lld -flto -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=LLDGARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -fuse-ld=lld -flto=thin -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=LLDGARANGE %s // // RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \ // RUN:| FileCheck -check-prefix=FDTS %s @@ -371,6 +375,8 @@ // NORNGBSE-NOT: -fdebug-ranges-base-address // // GARANGE-DAG: -generate-arange-section +// LDGARANGE-NOT: {{".*lld.*"}} {{.*}} "-generate-arange-section" +// LLDGARANGE-DAG: {{".*lld.*"}} {{.*}} "-generate-arange-section" // // FDTS: "-mllvm" "-generate-type-units" // FDTSE: error: unsupported option '-fdebug-types-section' for target 'x86_64-apple-darwin' Index: clang/lib/Driver/ToolChains/CommonArgs.cpp === --- clang/lib/Driver/ToolChains/CommonArgs.cpp +++ clang/lib/Driver/ToolChains/CommonArgs.cpp @@ -506,6 +506,14 @@ Suffix, Plugin); CmdArgs.push_back(Args.MakeArgString(Plugin)); + } else { +// NOTE: That it is not possible to use lld for PS4, hence no need to +// handle SCE (like in Clang.cpp::renderDebugOptions()) +bool NeedAranges = Args.hasArg(options::OPT_gdwarf_aranges); +if (NeedAranges) { + CmdArgs.push_back(Args.MakeArgString("-mllvm")); + CmdArgs.push_back(Args.MakeArgString("-generate-arange-section")); +} } // Try to pass driver level flags relevant to LTO code generation down to Index: clang/test/Driver/debug-options.c === --- clang/test/Driver/debug-options.c +++ clang/test/Driver/debug-options.c @@ -246,7 +246,11 @@ // RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=NOPUB %s // RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s // -// RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -flto -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=LDGARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -flto=thin -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=LDGARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -fuse-ld=lld -flto -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=LLDGARANGE %s +// RUN: %clang -### -target x86_64-unknown-linux -fuse-ld=lld -flto=thin -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=LLDGARANGE %s // // RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \ // RUN:| FileCheck -check-prefix=FDTS %s @@ -371,6 +375,8 @@ // NORNGBSE-NOT: -fdebug-ranges-base-address // // GARANGE-DAG: -generate-arange-section +// LDGARANGE-NOT: {{".*lld.*"}} {{.*}} "-generate-arange-section" +// LLDGARANGE-DAG: {{".*lld.*"}} {{.*}} "-generate-arange-section" // // FDTS: "-mllvm" "-generate-type-units" // FDTSE: error: unsupported option '-fdebug-types-section' for target 'x86_64-apple-darwin' Index: clang/lib/Driver/ToolChains/CommonArgs.cpp === --- clang/lib/Driver/ToolChains/CommonArgs.cpp +++ clang/lib/Driver/ToolChains/CommonArgs.cpp @@ -506,6 +506,14 @@ Suffix, Plugin); CmdArgs.push_back(Args.MakeArgString(Plugin)); + } else { +// NOTE: That it is not possible to use lld for PS4, hence no need to +// handle SCE (like in Clang.cpp::renderDebugOptions()) +bool NeedAranges = Args.hasArg(options::OPT_gdwarf_aranges); +if (NeedAranges) { + CmdArgs.push_back(Args.MakeArgString("-mllvm")); + CmdArgs.push_back(Args.M
Re: [clang] b7a7aee - [clang] Qualify auto in range-based for loops (NFC)
FWIW, sweeping NFC changes like this cause a fair amount of code churn (which makes tools like git blame a bit harder to use) for a relatively small improvement to code readability, which is why our developer policy asks that you "Avoid committing formatting- or whitespace-only changes outside of code you plan to make subsequent changes to." In the future, I'd caution against doing such large-scale sweeping NFC changes outside of areas you're actively working on unless there's some wider discussion with the community first. That said, all of your changes are all improvements, so thank you for them! Some of the changes you made would have ideally made it more clear when the deduced type is a pointer to a const object instead of hiding the qualifier behind the deduction. I've pointed out a couple such places below, but don't feel obligated to go back and change anything unless you're going to be touching other code in the area. ~Aaron On Sun, Sep 4, 2022 at 2:27 AM Kazu Hirata via cfe-commits wrote: > > > Author: Kazu Hirata > Date: 2022-09-03T23:27:27-07:00 > New Revision: b7a7aeee90cffefd0f73b8d9f44ab4d1d5123d05 > > URL: > https://github.com/llvm/llvm-project/commit/b7a7aeee90cffefd0f73b8d9f44ab4d1d5123d05 > DIFF: > https://github.com/llvm/llvm-project/commit/b7a7aeee90cffefd0f73b8d9f44ab4d1d5123d05.diff > > LOG: [clang] Qualify auto in range-based for loops (NFC) > > Added: > > > Modified: > clang/lib/ARCMigrate/ObjCMT.cpp > clang/lib/ARCMigrate/TransGCAttrs.cpp > clang/lib/AST/ASTContext.cpp > clang/lib/AST/ASTImporter.cpp > clang/lib/AST/Decl.cpp > clang/lib/AST/DeclObjC.cpp > clang/lib/AST/ODRHash.cpp > clang/lib/AST/OpenMPClause.cpp > clang/lib/AST/StmtProfile.cpp > clang/lib/AST/Type.cpp > clang/lib/Analysis/CFG.cpp > clang/lib/Analysis/ThreadSafetyCommon.cpp > clang/lib/CodeGen/CGBlocks.cpp > clang/lib/CodeGen/CGCall.cpp > clang/lib/CodeGen/CGClass.cpp > clang/lib/CodeGen/CGDebugInfo.cpp > clang/lib/CodeGen/CGDeclCXX.cpp > clang/lib/CodeGen/CGExpr.cpp > clang/lib/CodeGen/CGObjCGNU.cpp > clang/lib/CodeGen/CGObjCMac.cpp > clang/lib/CodeGen/CodeGenFunction.cpp > clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp > clang/lib/CodeGen/SwiftCallingConv.cpp > clang/lib/Driver/Compilation.cpp > clang/lib/Driver/Driver.cpp > clang/lib/Driver/ToolChains/Clang.cpp > clang/lib/Driver/ToolChains/CommonArgs.cpp > clang/lib/Driver/ToolChains/Gnu.cpp > clang/lib/Driver/ToolChains/HIPAMD.cpp > clang/lib/Format/Format.cpp > clang/lib/Frontend/FrontendActions.cpp > clang/lib/Index/USRGeneration.cpp > clang/lib/Sema/IdentifierResolver.cpp > clang/lib/Sema/Sema.cpp > clang/lib/Sema/SemaCodeComplete.cpp > clang/lib/Sema/SemaDecl.cpp > clang/lib/Sema/SemaDeclAttr.cpp > clang/lib/Sema/SemaDeclCXX.cpp > clang/lib/Sema/SemaDeclObjC.cpp > clang/lib/Sema/SemaExpr.cpp > clang/lib/Sema/SemaExprCXX.cpp > clang/lib/Sema/SemaInit.cpp > clang/lib/Sema/SemaLambda.cpp > clang/lib/Sema/SemaLookup.cpp > clang/lib/Sema/SemaObjCProperty.cpp > clang/lib/Sema/SemaOpenMP.cpp > clang/lib/Sema/SemaOverload.cpp > clang/lib/Sema/SemaTemplateDeduction.cpp > clang/lib/Sema/SemaTemplateInstantiateDecl.cpp > clang/lib/Serialization/ASTReader.cpp > clang/lib/Serialization/ASTWriterDecl.cpp > clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp > clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp > clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp > clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp > clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp > clang/lib/StaticAnalyzer/Core/CallEvent.cpp > clang/lib/StaticAnalyzer/Core/CoreEngine.cpp > clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp > clang/lib/Tooling/Tooling.cpp > > Removed: > > > > > diff --git a/clang/lib/ARCMigrate/ObjCMT.cpp > b/clang/lib/ARCMigrate/ObjCMT.cpp > index 26f751b77f86a..fe0ce4c5cdc3a 100644 > --- a/clang/lib/ARCMigrate/ObjCMT.cpp > +++ b/clang/lib/ARCMigrate/ObjCMT.cpp > @@ -792,7 +792,7 @@ static bool UseNSOptionsMacro(Preprocessor &PP, > ASTContext &Ctx, >bool PowerOfTwo = true; >bool AllHexdecimalEnumerator = true; >uint64_t MaxPowerOfTwoVal = 0; > - for (auto Enumerator : EnumDcl->enumerators()) { > + for (auto *Enumerator : EnumDcl->enumerators()) { > const Expr *InitExpr = Enumerator->getInitExpr(); > if (!InitExpr) { >PowerOfTwo = false; > > diff --git a/clang/lib/ARCMigrate/TransGCAttrs.cpp > b/clang/lib/ARCMigrate/TransGCAttrs.cpp > index 99a61e0842a76..b94aee2de573e 100644 > --- a/clang/lib/ARCMigrate/TransGCAttrs.cpp > +++ b/clang/lib/ARCMigrate/TransGCAttrs.cpp > @@ -158,7 +158,7 @@ class GCAttrsCollector : public > RecursiveASTVisitor { > if (!D) >
[PATCH] D133088: [Clang] Fix wrong diagnostic for scope identifier with internal linkage
junaire updated this revision to Diff 457855. junaire added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133088/new/ https://reviews.llvm.org/D133088 Files: clang/docs/ReleaseNotes.rst clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Sema/SemaDecl.cpp clang/test/Parser/cxx1z-decomposition.cpp clang/test/Sema/array-init.c clang/test/Sema/err-decl-block-extern-no-init.c clang/test/Sema/private-extern.c Index: clang/test/Sema/private-extern.c === --- clang/test/Sema/private-extern.c +++ clang/test/Sema/private-extern.c @@ -69,9 +69,9 @@ struct s0 { int x; }; void f9(void) { - extern int g15 = 0; // expected-error{{'extern' variable cannot have an initializer}} + extern int g15 = 0; // expected-error{{declaration of block scope identifier with external linkage shall have no initializer}} // FIXME: linkage specifier in warning. - __private_extern__ int g16 = 0; // expected-error{{'extern' variable cannot have an initializer}} + __private_extern__ int g16 = 0; // expected-error{{declaration of block scope identifier with external linkage shall have no initializer}} } extern int g17; Index: clang/test/Sema/err-decl-block-extern-no-init.c === --- /dev/null +++ clang/test/Sema/err-decl-block-extern-no-init.c @@ -0,0 +1,15 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s +static int x; + +void foo(void) +{ +extern int x = 1; // expected-error {{declaration of block scope identifier with internal linkage shall have no initializer}} +} + +int y; + +void bar(void) +{ +extern int y = 1; // expected-error {{declaration of block scope identifier with external linkage shall have no initializer}} + +} Index: clang/test/Sema/array-init.c === --- clang/test/Sema/array-init.c +++ clang/test/Sema/array-init.c @@ -48,7 +48,7 @@ struct threeElements *p = 7; // expected-error{{incompatible integer to pointer conversion initializing 'struct threeElements *' with an expression of type 'int'}} - extern int blockScopeExtern[3] = { 1, 3, 5 }; // expected-error{{'extern' variable cannot have an initializer}} + extern int blockScopeExtern[3] = { 1, 3, 5 }; // expected-error{{declaration of block scope identifier with external linkage shall have no initializer}} static long x2[3] = { 1.0, "abc", // expected-error{{incompatible pointer to integer conversion initializing 'long' with an expression of type 'char[4]'}} Index: clang/test/Parser/cxx1z-decomposition.cpp === --- clang/test/Parser/cxx1z-decomposition.cpp +++ clang/test/Parser/cxx1z-decomposition.cpp @@ -69,7 +69,7 @@ // storage-class-specifiers static auto &[a] = n; // expected-warning {{declared 'static' is a C++20 extension}} thread_local auto &[b] = n; // expected-warning {{declared 'thread_local' is a C++20 extension}} -extern auto &[c] = n; // expected-error {{cannot be declared 'extern'}} expected-error {{cannot have an initializer}} +extern auto &[c] = n; // expected-error {{cannot be declared 'extern'}} expected-error {{declaration of block scope identifier with external linkage shall have no initializer}} struct S { mutable auto &[d] = n; // expected-error {{not permitted in this context}} Index: clang/lib/Sema/SemaDecl.cpp === --- clang/lib/Sema/SemaDecl.cpp +++ clang/lib/Sema/SemaDecl.cpp @@ -12771,9 +12771,14 @@ return; } + // C99 6.7.8p5. C++ has no such restriction, but that is a defect. if (VDecl->isLocalVarDecl() && VDecl->hasExternalStorage()) { -// C99 6.7.8p5. C++ has no such restriction, but that is a defect. -Diag(VDecl->getLocation(), diag::err_block_extern_cant_init); +unsigned LinkageKind = /*external*/ 0; +// C2x 6.7.10p6. +if (VDecl->getFormalLinkage() == InternalLinkage) + LinkageKind = /*internal*/ 1; + +Diag(VDecl->getLocation(), diag::err_block_extern_cant_init) << LinkageKind; VDecl->setInvalidDecl(); return; } Index: clang/include/clang/Basic/DiagnosticSemaKinds.td === --- clang/include/clang/Basic/DiagnosticSemaKinds.td +++ clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -5901,7 +5901,8 @@ : Error<"variable %0 cannot be declared both 'extern' and with the " "'loader_uninitialized' attribute">; def err_block_extern_cant_init : Error< - "'extern' variable cannot have an initializer">; + "declaration of block scope identifier with %select{external|internal}0 " + "linkage shall have no initializer">; def warn_extern_init : Warning<"'extern' variable has an initializer">,
[PATCH] D132131: [clang-format] Adds a formatter for aligning trailing comments over empty lines
yusuke-kadowaki updated this revision to Diff 457864. yusuke-kadowaki marked 8 inline comments as done. yusuke-kadowaki added a comment. - Revert doc - Revert rst as well - Apply format - Update implementation to deal with the setting of MaxEmptyLinesToKeep - Add test for the combination with MaxEmptyLinesToKeep Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132131/new/ https://reviews.llvm.org/D132131 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/lib/Format/WhitespaceManager.cpp clang/unittests/Format/FormatTest.cpp clang/unittests/Format/FormatTestComments.cpp Index: clang/unittests/Format/FormatTestComments.cpp === --- clang/unittests/Format/FormatTestComments.cpp +++ clang/unittests/Format/FormatTestComments.cpp @@ -2858,6 +2858,200 @@ "int a; //\n"); } +TEST_F(FormatTestComments, AlignTrailingCommentsAcrossEmptyLines) { + FormatStyle Style = getLLVMStyle(); + Style.AlignConsecutiveTrailingComments.AcrossEmptyLines = true; + verifyFormat("#include \"a.h\" // simple\n" + "\n" + "#include \"aa.h\" // example case\n", + Style); + + verifyFormat("#include \"a.h\" // align across\n" + "\n" + "#include \"aa.h\" // two empty lines\n" + "\n" + "#include \"aaa.h\" // in a row\n", + Style); + + verifyFormat("#include \"a.h\" // align\n" + "#include \"aa.h\" // comment\n" + "#include \"aaa.h\"// blocks\n" + "\n" + "#include \".h\" // across\n" + "#include \"a.h\" // one\n" + "#include \"aa.h\" // empty line\n", + Style); + + verifyFormat("#include \"a.h\" // align trailing comments\n" + "#include \"a.h\"\n" + "#include \"aa.h\" // across a line without comment\n", + Style); + + verifyFormat("#include \"a.h\" // align across\n" + "#include \"a.h\"\n" + "#include \"aa.h\" // two lines without comment\n" + "#include \"a.h\"\n" + "#include \"aaa.h\" // in a row\n", + Style); + + verifyFormat("#include \"a.h\" // align\n" + "#include \"aa.h\" // comment\n" + "#include \"aaa.h\"// blocks\n" + "#include \"a.h\"\n" + "#include \".h\" // across\n" + "#include \"a.h\" // a line without\n" + "#include \"aa.h\" // comment\n", + Style); + + // Start of testing the combination with MaxEmptyLinesToKeep + Style.MaxEmptyLinesToKeep = 0; + EXPECT_EQ("int a; // comment\n" +"int ab; // comment\n" +"int abc; // comment\n", +format("int a; // comment\n" + "int ab; // comment\n" + "\n" + "int abc; // comment\n", + Style)); + + Style.MaxEmptyLinesToKeep = 1; + EXPECT_EQ("int a; // comment\n" +"int ab; // comment\n" +"\n" +"int abc; // comment\n", +format("int a; // comment\n" + "int ab; // comment\n" + "\n" + "\n" + "int abc; // comment\n", + Style)); + + Style.MaxEmptyLinesToKeep = 2; + EXPECT_EQ("int a; // comment\n" +"int ab; // comment\n" +"\n" +"\n" +"int abc; // comment\n", +format("int a; // comment\n" + "int ab; // comment\n" + "\n" + "\n" + "\n" + "int abc; // comment\n", + Style)); + + // Reset the setting + Style.MaxEmptyLinesToKeep = 1; + // End of testing the combination with MaxEmptyLinesToKeep + + Style.ColumnLimit = 15; + EXPECT_EQ("int ab; // line\n" +"int a; // long\n" +"// long\n" +"\n" +"// long", +format("int ab; // line\n" + "int a; // long long\n" + "\n" + "// long", + Style)); + + Style.ColumnLimit = 15; + EXPECT_EQ("int ab; // line\n" +"\n" +"int a; // long\n" +"// long\n", +format("int ab; // line\n" + "\n" + "int a; // long long\n", + Style)); + + Style.ColumnLimit = 30; + EXPECT_EQ("int foo = 12345; // comment\n" +"int bar =\n" +"1234; // This is a very\n" +" // long comment\n" +" // which is wrapped\n" +" // arround.\n" +"
[PATCH] D132131: [clang-format] Adds a formatter for aligning trailing comments over empty lines
yusuke-kadowaki added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:3698 - QualifierOrder: ['inline', 'static', 'type', 'const'] + QualifierOrder: ['inline', 'static' , 'type', 'const'] HazardyKnusperkeks wrote: > Anyone knows where this comes from? > You are using the script to generate the rst, right? Yes, I'm using `dump_format_style.py`. Comment at: clang/include/clang/Format/Format.h:154 /// \code /// AlignConsecutiveMacros: AcrossEmptyLines /// HazardyKnusperkeks wrote: > yusuke-kadowaki wrote: > > HazardyKnusperkeks wrote: > > > The change/addition has to be here, since here it directly states > > > `AlignConsecutiveMacros`. > > What are you meaning to say here? I thought saying `AlignConsecutiveStyle` > > is used for multiple options is what we are trying to do. > > > > Should we say something like, > > > Here AlignConsecutiveMacros is used as an example, but in practice > > > AlignConsecutiveStyle is also used with other options. > > in this place? > The example mentions explicitly only `AlignConsecutiveMacros` which is a bit > misleading if you are only looking at the documentation of > `AlignConsecutiveAssignments` for example. > > This is not your fault, and I'm fine with ignoring it here. A separate patch > to fix that is welcome. :) Got it. Reverted for now. Comment at: clang/lib/Format/WhitespaceManager.cpp:930 unsigned Newlines = 0; + unsigned int NewLineThreshold = Style.AlignConsecutiveTrailingComments.AcrossEmptyLines ? 2 : 1; for (unsigned i = 0, e = Changes.size(); i != e; ++i) { HazardyKnusperkeks wrote: > And accompanied by a short test. That should be everything to support the > mixture of the options, right? > That should be everything to support the mixture of the options, right? I think so. Done. Comment at: clang/lib/Format/WhitespaceManager.cpp:984 +} +else if (BreakBeforeNext || Newlines > NewLineThreshold || (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn) || HazardyKnusperkeks wrote: > Is this clang-format formatted? I forgot that. Also formatted some other diffs. Comment at: clang/unittests/Format/FormatTestComments.cpp:4262 /\ -/ +/ )", HazardyKnusperkeks wrote: > Please revert this. Reverted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132131/new/ https://reviews.llvm.org/D132131 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] a46154c - [analyzer] Warn if the size of the array in `new[]` is undefined
Author: isuckatcs Date: 2022-09-04T23:06:58+02:00 New Revision: a46154cb1cd09aa26bc803d8696e6e9283aac6a9 URL: https://github.com/llvm/llvm-project/commit/a46154cb1cd09aa26bc803d8696e6e9283aac6a9 DIFF: https://github.com/llvm/llvm-project/commit/a46154cb1cd09aa26bc803d8696e6e9283aac6a9.diff LOG: [analyzer] Warn if the size of the array in `new[]` is undefined This patch introduces a new checker, called NewArraySize checker, which detects if the expression that yields the element count of the array in new[], results in an Undefined value. Differential Revision: https://reviews.llvm.org/D131299 Added: clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp clang/test/Analysis/undefined-new-element.cpp Modified: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp clang/test/Analysis/Issue56873.cpp Removed: diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 623a520574e2..2903b3d46441 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -245,6 +245,22 @@ Check for uninitialized values being returned to the caller. return x; // warn } +.. _core-uninitialized-NewArraySize: + +core.uninitialized.NewArraySize (C++) +" + +Check if the element count in new[] is garbage or undefined. + +.. code-block:: cpp + + void test() { +int n; +int *arr = new int[n]; // warn: Element count in new[] is a garbage value +delete[] arr; + } + + .. _cplusplus-checkers: diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index cf8cec3b13c3..3dd2c7c1606c 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -437,6 +437,10 @@ def ReturnUndefChecker : Checker<"UndefReturn">, HelpText<"Check for uninitialized values being returned to the caller">, Documentation; +def UndefinedNewArraySizeChecker : Checker<"NewArraySize">, + HelpText<"Check if the size of the array in a new[] expression is undefined">, + Documentation; + } // end "core.uninitialized" //===--===// diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h index 50a27a211ef0..42a51b4c8e9e 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h @@ -1033,6 +1033,18 @@ class CXXAllocatorCall : public AnyFunctionCall { return getOriginExpr()->getNumPlacementArgs() + getNumImplicitArgs(); } + bool isArray() const { return getOriginExpr()->isArray(); } + + Optional getArraySizeExpr() const { +return getOriginExpr()->getArraySize(); + } + + SVal getArraySizeVal() const { +assert(isArray() && "The allocator call doesn't allocate and array!"); + +return getState()->getSVal(*getArraySizeExpr(), getLocationContext()); + } + const Expr *getArgExpr(unsigned Index) const override { // The first argument of an allocator call is the size of the allocation. if (Index < getNumImplicitArgs()) diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 84886b93d2e4..c6be8fe21288 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -120,6 +120,7 @@ add_clang_library(clangStaticAnalyzerCheckers UndefResultChecker.cpp UndefinedArraySubscriptChecker.cpp UndefinedAssignmentChecker.cpp + UndefinedNewArraySizeChecker.cpp UninitializedObject/UninitializedObjectChecker.cpp UninitializedObject/UninitializedPointee.cpp UnixAPIChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index b281d9b267e8..5613b8f7b4c9 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -1733,6 +1733,10 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, // Fill the region with the initialization value. State = State->bindDefaultInitial(RetVal, Init, LCtx); + // If Size is somehow undefined at this point, this line prevents a crash. + if (Size.isUndef()) +Size = UnknownVal(); + // Set the region's extent. State = setDynamicExtent(State, RetVal.getAsRegion(), Size.castAs(), svalBuilder); diff --git a/clang/lib/StaticAnalyzer/Checkers/Unde
[PATCH] D131299: [analyzer] Warn if the size of the array in `new[]` is undefined
This revision was automatically updated to reflect the committed changes. Closed by commit rGa46154cb1cd0: [analyzer] Warn if the size of the array in `new[]` is undefined (authored by isuckatcs). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131299/new/ https://reviews.llvm.org/D131299 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp clang/test/Analysis/Issue56873.cpp clang/test/Analysis/undefined-new-element.cpp Index: clang/test/Analysis/undefined-new-element.cpp === --- /dev/null +++ clang/test/Analysis/undefined-new-element.cpp @@ -0,0 +1,69 @@ +// RUN: %clang_analyze_cc1 %s -analyzer-checker=core.uninitialized.NewArraySize -analyzer-output=text -verify + +#include "Inputs/system-header-simulator-cxx.h" + +void checkUndefinedElmenetCountValue() { + int n; + // expected-note@-1{{'n' declared without an initial value}} + + int *arr = new int[n]; // expected-warning{{Element count in new[] is a garbage value}} + // expected-note@-1{{Element count in new[] is a garbage value}} +} + +void checkUndefinedElmenetCountMultiDimensionalValue() { + int n; + // expected-note@-1{{'n' declared without an initial value}} + + auto *arr = new int[n][5]; // expected-warning{{Element count in new[] is a garbage value}} + // expected-note@-1{{Element count in new[] is a garbage value}} +} + +void checkUndefinedElmenetCountReference() { + int n; + // expected-note@-1{{'n' declared without an initial value}} + + int &ref = n; + // expected-note@-1{{'ref' initialized here}} + + int *arr = new int[ref]; // expected-warning{{Element count in new[] is a garbage value}} + // expected-note@-1{{Element count in new[] is a garbage value}} +} + +void checkUndefinedElmenetCountMultiDimensionalReference() { + int n; + // expected-note@-1{{'n' declared without an initial value}} + + int &ref = n; + // expected-note@-1{{'ref' initialized here}} + + auto *arr = new int[ref][5]; // expected-warning{{Element count in new[] is a garbage value}} + // expected-note@-1{{Element count in new[] is a garbage value}} +} + +int foo() { + int n; + + return n; +} + +void checkUndefinedElmenetCountFunction() { + int *arr = new int[foo()]; // expected-warning{{Element count in new[] is a garbage value}} + // expected-note@-1{{Element count in new[] is a garbage value}} +} + +void checkUndefinedElmenetCountMultiDimensionalFunction() { + auto *arr = new int[foo()][5]; // expected-warning{{Element count in new[] is a garbage value}} + // expected-note@-1{{Element count in new[] is a garbage value}} +} + +void *malloc(size_t); + +void checkUndefinedPlacementElementCount() { + int n; + // expected-note@-1{{'n' declared without an initial value}} + + void *buffer = malloc(sizeof(std::string) * 10); + std::string *p = + ::new (buffer) std::string[n]; // expected-warning{{Element count in new[] is a garbage value}} + // expected-note@-1{{Element count in new[] is a garbage value}} +} Index: clang/test/Analysis/Issue56873.cpp === --- clang/test/Analysis/Issue56873.cpp +++ clang/test/Analysis/Issue56873.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s +// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s void clang_analyzer_warnIfReached(); Index: clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp === --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp @@ -0,0 +1,80 @@ +//===--- UndefinedNewArraySizeChecker.cpp ---*- C++ -*--==// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===--===// +// +// This defines UndefinedNewArraySizeChecker, a builtin check in ExprEngine +// that checks if the size of the array in a new[] expression is undefined. +// +//===--===// + +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticA
[PATCH] D132713: [clang-tidy] Skip union-like classes in use-equals-default
alexander-shaposhnikov added a comment. gentle ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132713/new/ https://reviews.llvm.org/D132713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128750: [C++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions
mizvekov added inline comments. Comment at: clang/include/clang/Sema/SemaConcept.h:48-52 + if (ArgA.getKind() == TemplateArgument::Expression && + ArgB.getKind() == TemplateArgument::Expression && + ArgA.getAsExpr()->getType()->isUndeducedAutoType() && + ArgB.getAsExpr()->getType()->isUndeducedAutoType()) +continue; Why are looking at only the type of the expression? The expression can be arbitrarily different, but as long as they are both undeduced auto, that is okay? Comment at: clang/include/clang/Sema/SemaConcept.h:54-68 + if (ArgA.getKind() == TemplateArgument::Type && + ArgB.getKind() == TemplateArgument::Type) +if (const auto *SubstA = +ArgA.getAsType()->getAs()) + if (const auto *SubstB = + ArgB.getAsType()->getAs()) { +QualType ReplacementA = SubstA->getReplacementType(); It's a bit odd to find `SubstTemplateTypeParmType` necessary to implement the semantics of this change. This is just type sugar we leave behind in the template instantiator to mark where type replacement happened. There are several potential issues here: 1) This could be marking a substitution that happened at any template depth. Ie this could be marking a substitution from an outer template. Does the level not matter here at all? 2) If the level does matter, you won't be able to reconstitute that easily without further improvements. See https://github.com/llvm/llvm-project/issues/55886 for example. 3) A substitution can replace a dependent type for another one, and when that other gets replaced, we lose track of that because `SubstTemplateTypeParmType` only holds a canonical underlying type. Leaving that aside, I get the impression you are trying to work around the fact that when an expression appears in a canonical type, presumably because that expression is dependent on an NTTP, we can't rely on uniquing anymore to compare if they are the same type, as we lack in Clang the equivalent concept of canonicalization for expressions. But this however is a bit hard to implement. Are we sure the standard requires this, or can we simply consider these types always different? Comment at: clang/lib/AST/ASTContext.cpp:5149-5151 +Expr *E = new (*this) +DeclRefExpr(*this, NTTP, /*enclosing*/ false, T, +Expr::getValueKindForType(NTTPType), NTTP->getLocation()); Comment at: clang/lib/AST/ASTContext.cpp:5154-5156 + E = new (*this) PackExpansionExpr( + NTTPType->isUndeducedAutoType() ? NTTPType : DependentTy, E, + NTTP->getLocation(), None); I don't know if this change is necessary for this patch, as this looks part of the workaround in `SemaConcept.h`, but I think a better way to preserve the type here might be to always use `NTTPType`, but then add an additional `Dependent` parameter to `PackExpansionExpr` which can be used to force the expression to be dependent. Comment at: clang/lib/Sema/SemaConcept.cpp:827 +assert(FoldE->isRightFold() && FoldE->getOperator() == BO_LAnd); +E = FoldE->getPattern(); + } If you need to dig down into the pattern, then I think the pattern might also contain casts and parenthesis which you would need to keep ignoring for the rest of the code to work. I would consider adding a test for that. Comment at: clang/lib/Sema/SemaOverload.cpp:9675-9676 - // when comparing template functions. - if (Cand1.Function && Cand2.Function && Cand1.Function->hasPrototype() && - Cand2.Function->hasPrototype()) { auto *PT1 = cast(Cand1.Function->getFunctionType()); Why are these `hasPrototype` checks not needed anymore? I don't see other changes which would obliviate the need for it. Presumably the code below is assuming we have them when we perform that `FunctionProtoType` cast. Maybe this was either not possible, or we simply never added tests for it. Comment at: clang/lib/Sema/SemaTemplate.cpp:1267 BuildDeclRefExpr(NTTP, NTTP->getType(), VK_PRValue, NTTP->getLocation()); - if (!Ref) -return true; + assert(Ref); ExprResult ImmediatelyDeclaredConstraint = formImmediatelyDeclaredConstraint( I don't think the `assert` is even necessary TBH, the function can't return nullptr. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5247-5248 + for (unsigned i = 0; i < NumParams1; ++i) +if (FD1->getParamDecl(i)->getType().getCanonicalType() != +FD2->getParamDecl(i)->getType().getCanonicalType()) + return nullptr; Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5256-5257 + // than the other. + if (TPOC == TPOC_Conversion && FD1->getReturnType().getCanonicalType() != +
[clang] baa9eae - [NFC] fix incorrect indentation in docs
Author: Chuanqi Xu Date: 2022-09-05T11:05:23+08:00 New Revision: baa9eae279c1639f406015734ebbf4c429b15c21 URL: https://github.com/llvm/llvm-project/commit/baa9eae279c1639f406015734ebbf4c429b15c21 DIFF: https://github.com/llvm/llvm-project/commit/baa9eae279c1639f406015734ebbf4c429b15c21.diff LOG: [NFC] fix incorrect indentation in docs Added: Modified: clang/docs/StandardCPlusPlusModules.rst Removed: diff --git a/clang/docs/StandardCPlusPlusModules.rst b/clang/docs/StandardCPlusPlusModules.rst index ae434b14ef50c..86ba6f44dbb02 100644 --- a/clang/docs/StandardCPlusPlusModules.rst +++ b/clang/docs/StandardCPlusPlusModules.rst @@ -481,19 +481,19 @@ Then it is problematic if we remove ``foo.h`` before import `foo` module. .. code-block:: console - clang++ -std=c++20 foo.cppm --precompile -o foo.pcm - mv foo.h foo.orig.h + $ clang++ -std=c++20 foo.cppm --precompile -o foo.pcm + $ mv foo.h foo.orig.h # The following one is rejected - clang++ -std=c++20 Use.cpp -fmodule-file=foo.pcm -c + $ clang++ -std=c++20 Use.cpp -fmodule-file=foo.pcm -c The above case will rejected. And we're still able to workaround it by ``-Xclang -fmodules-embed-all-files`` option: .. code-block:: console - clang++ -std=c++20 foo.cppm --precompile -Xclang -fmodules-embed-all-files -o foo.pcm - mv foo.h foo.orig.h - clang++ -std=c++20 Use.cpp -fmodule-file=foo.pcm -c -o Use.o - clang++ Use.o foo.pcm + $ clang++ -std=c++20 foo.cppm --precompile -Xclang -fmodules-embed-all-files -o foo.pcm + $ mv foo.h foo.orig.h + $ clang++ -std=c++20 Use.cpp -fmodule-file=foo.pcm -c -o Use.o + $ clang++ Use.o foo.pcm ABI Impacts --- ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132192: [RISCV] Add '32bit' feature to rv32 only builtins.
craig.topper added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:4359 -return Diag(TheCall->getCallee()->getBeginLoc(), -diag::err_32_bit_builtin_64_bit_tgt); - luismarques wrote: > That tablegen def is still being used for X86. Maybe you could make a similar > patch for X86? There's no "32bit" feature in X86.td in the backend. This was possible for RISC-V because I added "32bit" when I refactored mtune recently. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132192/new/ https://reviews.llvm.org/D132192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132952: [Sema] disable -Wvla for function array parameters
inclyc added inline comments. Comment at: clang/test/Sema/warn-vla.c:8-12 +void test2(int n, int v[n]) { // c99 no-warning +#if __STDC_VERSION__ < 199901L +// expected-warning@-2{{variable length arrays are a C99 feature}} +#endif } aaron.ballman wrote: > The diagnostic there is rather unfortunate because we're not using a > variable-length array in this case. Emm, I'm not clear about whether we should consider this a VLA, and generates `-Wvla-extensions`. Is `v[n]` literally a variable-length array? (in source code) So it seems to me that we should still report c89 incompatibility warnings? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132952/new/ https://reviews.llvm.org/D132952 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] 36a1ca5 - [ASTReader] Fix -Wunused-private-field in non-assertion builds after D128490. NFC
Author: Fangrui Song Date: 2022-09-04T23:48:55-07:00 New Revision: 36a1ca5835e0f7e0e02899d97cd2e4c7bf704361 URL: https://github.com/llvm/llvm-project/commit/36a1ca5835e0f7e0e02899d97cd2e4c7bf704361 DIFF: https://github.com/llvm/llvm-project/commit/36a1ca5835e0f7e0e02899d97cd2e4c7bf704361.diff LOG: [ASTReader] Fix -Wunused-private-field in non-assertion builds after D128490. NFC Added: Modified: clang/lib/Serialization/ASTReader.cpp Removed: diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 5d1ee45c10de5..568d948b541e5 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9818,6 +9818,7 @@ void ASTReader::diagnoseOdrViolations() { } assert(Context.hasSameType(FirstField->getType(), SecondField->getType())); +(void)Context; QualType FirstType = FirstField->getType(); QualType SecondType = SecondField->getType(); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132607: [OffloadPackager] Add ability to extract images from other file types
saiislam accepted this revision. saiislam added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132607/new/ https://reviews.llvm.org/D132607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits