[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
courbet added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:155 + if (!LhsIntegerRange.Contains(IntegerConstant)) +diag(SourceLoc, "narrowing conversion from %0 to %1") << RhsType << LhsType; + return true; I think it would be clearer to have something like "narrowing conversion from %0 literal to %1" Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:163 + int i; + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] I think some people would not like to be warned on this (especially for the form `if (!returns_int())`, because the `!` does the cast). What about adding options to the check to disable some forms ? Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } What about providing a fix for this one : `while (i != 0) {` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
lebedev.ri added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } courbet wrote: > What about providing a fix for this one : > `while (i != 0) {` Is this //actually// a narrowing conversion as per the CPPCG? Conversion to bool is a special case, it's not a truncation, but a `x != 0`. I'd think cases like these where bool was pretty clearly meant (`if()`, `while()`, `for()`, ???) should be exempt. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.
hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. The change looks reasonable to me. In https://reviews.llvm.org/D54061#1286505, @steveire wrote: > After this change, it seems that `ClangTidyCheck::check` is not needed and > all callers should be ported to call `run()` instead (and the private `run()` > declaration should be removed from `ClangTidyCheck`. Theoretically, we could replace `ClangTidyCheck::check` with `ClangTidyCheck::run`, but I'm not sure it is worth, `ClangTidyCheck::check` is a public API, and is widely-used (for all clang-tidy checks), replacing it requires large changes (although it is one-line change), it might break downstream clang-tidy checks. Comment at: clang-tidy/ClangTidy.cpp:444 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) { - Context->setSourceManager(Result.SourceManager); check(Result); I'd add an assertion `assert(Context->hasSourceManager())`, but it turns out ClangTidyContext doesn't provide this method... Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.
JonasToth added a comment. > Theoretically, we could replace `ClangTidyCheck::check` with > `ClangTidyCheck::run`, but I'm not sure it is worth, `ClangTidyCheck::check` > is a public API, and is widely-used (for all clang-tidy checks), replacing it > requires large changes (although it is one-line change), it might break > downstream clang-tidy checks. We can add a deprecation warning and remove the `check` method in the next version? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions
xbolva00 added a comment. Ping :) https://reviews.llvm.org/D52835 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54091: [RISCV] Add inline asm constraints I, J & K for RISC-V
lewis-revill created this revision. lewis-revill added a reviewer: asb. Herald added subscribers: cfe-commits, jocewei, PkmX, rkruppe, the_o, brucehoult, MartinMosbeck, rogfer01, mgrang, edward-jones, zzheng, jrtc27, shiva0217, kito-cheng, niosHD, sabuasal, apazos, simoncook, johnrusso, rbar, eraman. This allows the constraints I, J & K to be used in inline asm for RISC-V, with the following semantics (equivalent to GCC): I: Any 12-bit signed immediate. J: Immediate integer zero only. K: Any 5-bit unsigned immediate. Note that GCC also implements 'f' for floating point register and 'A' for address-only operand. These are not implemented here because: 1. It appears trivial to implement the floating point register constraint, however since floating point registers are not recognised by the calling convention the call to the inline asm node cannot be lowered. 2. I'm not yet certain how to implement an 'address-only' operand and I'd rather get the above constraints done first and add it later. Repository: rC Clang https://reviews.llvm.org/D54091 Files: lib/Basic/Targets/RISCV.cpp lib/Basic/Targets/RISCV.h test/CodeGen/riscv-inline-asm.c Index: test/CodeGen/riscv-inline-asm.c === --- /dev/null +++ test/CodeGen/riscv-inline-asm.c @@ -0,0 +1,28 @@ +// RUN: %clang_cc1 -triple riscv32 -target-feature +f -O2 -emit-llvm %s -o - \ +// RUN: | FileCheck %s +// RUN: %clang_cc1 -triple riscv64 -target-feature +f -O2 -emit-llvm %s -o - \ +// RUN: | FileCheck %s + +// Test RISC-V specific inline assembly constraints. + +void test_I() { +// CHECK-LABEL: define void @test_I() +// CHECK: call void asm sideeffect "", "I"(i32 2047) + asm volatile ("" :: "I"(2047)); +// CHECK: call void asm sideeffect "", "I"(i32 -2048) + asm volatile ("" :: "I"(-2048)); +} + +void test_J() { +// CHECK-LABEL: define void @test_J() +// CHECK: call void asm sideeffect "", "J"(i32 0) + asm volatile ("" :: "J"(0)); +} + +void test_K() { +// CHECK-LABEL: define void @test_K() +// CHECK: call void asm sideeffect "", "K"(i32 31) + asm volatile ("" :: "K"(31)); +// CHECK: call void asm sideeffect "", "K"(i32 0) + asm volatile ("" :: "K"(0)); +} Index: lib/Basic/Targets/RISCV.h === --- lib/Basic/Targets/RISCV.h +++ lib/Basic/Targets/RISCV.h @@ -62,9 +62,7 @@ ArrayRef getGCCRegAliases() const override; bool validateAsmConstraint(const char *&Name, - TargetInfo::ConstraintInfo &Info) const override { -return false; - } + TargetInfo::ConstraintInfo &Info) const override; bool hasFeature(StringRef Feature) const override; Index: lib/Basic/Targets/RISCV.cpp === --- lib/Basic/Targets/RISCV.cpp +++ lib/Basic/Targets/RISCV.cpp @@ -40,6 +40,26 @@ return llvm::makeArrayRef(GCCRegAliases); } +bool RISCVTargetInfo::validateAsmConstraint( +const char *&Name, TargetInfo::ConstraintInfo &Info) const { + switch (*Name) { + default: +return false; + case 'I': +// A 12-bit signed immediate. +Info.setRequiresImmediate(-2048, 2048); +return true; + case 'J': +// Integer zero. +Info.setRequiresImmediate(0); +return true; + case 'K': +// A 5-bit unsigned immediate for CSR access instructions. +Info.setRequiresImmediate(0, 31); +return true; + } +} + void RISCVTargetInfo::getTargetDefines(const LangOptions &Opts, MacroBuilder &Builder) const { Builder.defineMacro("__ELF__"); Index: test/CodeGen/riscv-inline-asm.c === --- /dev/null +++ test/CodeGen/riscv-inline-asm.c @@ -0,0 +1,28 @@ +// RUN: %clang_cc1 -triple riscv32 -target-feature +f -O2 -emit-llvm %s -o - \ +// RUN: | FileCheck %s +// RUN: %clang_cc1 -triple riscv64 -target-feature +f -O2 -emit-llvm %s -o - \ +// RUN: | FileCheck %s + +// Test RISC-V specific inline assembly constraints. + +void test_I() { +// CHECK-LABEL: define void @test_I() +// CHECK: call void asm sideeffect "", "I"(i32 2047) + asm volatile ("" :: "I"(2047)); +// CHECK: call void asm sideeffect "", "I"(i32 -2048) + asm volatile ("" :: "I"(-2048)); +} + +void test_J() { +// CHECK-LABEL: define void @test_J() +// CHECK: call void asm sideeffect "", "J"(i32 0) + asm volatile ("" :: "J"(0)); +} + +void test_K() { +// CHECK-LABEL: define void @test_K() +// CHECK: call void asm sideeffect "", "K"(i32 31) + asm volatile ("" :: "K"(31)); +// CHECK: call void asm sideeffect "", "K"(i32 0) + asm volatile ("" :: "K"(0)); +} Index: lib/Basic/Targets/RISCV.h === --- lib/Basic/Targets/RISCV.h +++ lib/Basic/Targets/RISCV.h @@ -62,9 +62,7 @@ ArrayRef getGCCRegAliases() const override; bool validateAsmConstraint(const cha
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
hokein created this revision. hokein added a reviewer: ioeric. We can run the tools on a subset files of compilation database. Repository: rC Clang https://reviews.llvm.org/D54092 Files: lib/Tooling/AllTUsExecution.cpp Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,11 @@ } // namespace +static llvm::cl::opt Filter( +"filter", +llvm::cl::desc("Only process files that match this filter"), +llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +115,10 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,11 @@ } // namespace +static llvm::cl::opt Filter( +"filter", +llvm::cl::desc("Only process files that match this filter"), +llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +115,10 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } lebedev.ri wrote: > courbet wrote: > > What about providing a fix for this one : > > `while (i != 0) {` > Is this //actually// a narrowing conversion as per the CPPCG? > Conversion to bool is a special case, it's not a truncation, but a `x != 0`. > I'd think cases like these where bool was pretty clearly meant (`if()`, > `while()`, `for()`, ???) should be exempt. No it is not an narrowing conversion the CPPCG are concerned: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions Short: ``` ES.46: Avoid lossy (narrowing, truncating) arithmetic conversions Enforcement A good analyzer can detect all narrowing conversions. However, flagging all narrowing conversions will lead to a lot of false positives. Suggestions: - flag all floating-point to integer conversions (maybe only float->char and double->int. Here be dragons! we need data) - flag all long->char (I suspect int->char is very common. Here be dragons! we need data) - consider narrowing conversions for function arguments especially suspect ``` We do have a readability-check that is concerned about int -> bool conversions. Because of that I think we could safely ignore the `int->bool` conversion. But `float -> bool` is still suspicious in my opinion. It has the same semantics, but comparing `float` on equality is dangerous and one could argue it shall be enforced through point 1 (all float-to-int conversions). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53701: [Analyzer] Record and process comparison of symbols instead of iterator positions in interator checkers
baloghadamsoftware added a comment. Your suggestion sounds completely reasonable. We cannot rely entirely on inlining of comparison operators. However, there is an important side-effect of inlining: on iterator-adapters inlining ensures that we handle the comparison of the underlying iterator correctly. Without inlining, `evalCall()` only works on the outermost iterator which is not always correct: it may happen that the checker cannot conclude any useful relation between the two iterator-adapters but there are some relations stored about the inner iterators. Based on my experiments quite many of the false-positives are related to iterator-adapters. So I am afraid that we introduce even more false-positives by losing inlining. I wonder whether the current "mixed" approach introduces additional paths because we do not do explicit state-splits and function `processComparison()` removes contradicting branches. Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:2066 "Symbol comparison must be a comparison"); return assumeNoOverflow(NewState, cast(CompSym)->getLHS(), 2); } NoQ wrote: > P.S. What was the idea here? Like, `CompSym` was just computed via `BO_EQ` > and has type of a condition, i.e. `bool` because we are in C++. Is this code > trying to say that the result of the comparison is bounded by `true/2`? There is also a `->getLHS()` which means that we enforce the bound on the left-hand side of the rearranged comparison. Although both symbols are bounded by `max/4`, constraint manager does not imply that the sum/diff is the bounded by `max/2`. I have to enforce this manually to prevent `min` negated to `min` when the constraint manager negates the difference. Repository: rC Clang https://reviews.llvm.org/D53701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
ioeric added inline comments. Comment at: lib/Tooling/AllTUsExecution.cpp:58 +"filter", +llvm::cl::desc("Only process files that match this filter"), +llvm::cl::init(".*")); Please also mention that this only applies to all-TUs. Comment at: lib/Tooling/AllTUsExecution.cpp:120 for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; > `Filter.getNumOccurrences() != 0 ` Would this work if `Filter` is set grammatically? Although it's not exposed now, it might make more sense to check `Filter != ".*"`. Repository: rC Clang https://reviews.llvm.org/D54092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346123 - Fix breakage on FrontendTest by initializing new field on constructor
Author: kadircet Date: Mon Nov 5 02:01:34 2018 New Revision: 346123 URL: http://llvm.org/viewvc/llvm-project?rev=346123&view=rev Log: Fix breakage on FrontendTest by initializing new field on constructor Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h?rev=346123&r1=346122&r2=346123&view=diff == --- cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h (original) +++ cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h Mon Nov 5 02:01:34 2018 @@ -249,12 +249,11 @@ private: public: AnalyzerOptions() : DisableAllChecks(false), ShowCheckerHelp(false), -ShowEnabledCheckerList(false), AnalyzeAll(false), -AnalyzerDisplayProgress(false), AnalyzeNestedBlocks(false), -eagerlyAssumeBinOpBifurcation(false), TrimGraph(false), -visualizeExplodedGraphWithGraphViz(false), -UnoptimizedCFG(false), -PrintStats(false), NoRetryExhausted(false) {} +ShowEnabledCheckerList(false), ShowConfigOptionsList(false), +AnalyzeAll(false), AnalyzerDisplayProgress(false), +AnalyzeNestedBlocks(false), eagerlyAssumeBinOpBifurcation(false), +TrimGraph(false), visualizeExplodedGraphWithGraphViz(false), +UnoptimizedCFG(false), PrintStats(false), NoRetryExhausted(false) {} /// Interprets an option's string value as a boolean. The "true" string is /// interpreted as true and the "false" string is interpreted as false. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r345989 - [analyzer] New flag to print all -analyzer-config options
Should be fixed in r346123. +Kadir Çetinkaya who authored the fix. On Fri, Nov 2, 2018 at 7:38 PM Galina Kistanova via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Also one of your resent commits added another broken test to the builder: > > http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/13729 > . . . > Failing Tests: > . . . > Clang-Unit :: > Frontend/./FrontendTests.exe/FrontendOutputTests.TestOutputStream > . . . > > The builder did not send notifications on this. > Please have a look? > > Thanks > > Galina > > On Fri, Nov 2, 2018 at 9:01 AM Kristof Umann via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: szelethus >> Date: Fri Nov 2 08:59:37 2018 >> New Revision: 345989 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=345989&view=rev >> Log: >> [analyzer] New flag to print all -analyzer-config options >> >> A new -cc1 flag is avaible for the said purpose: -analyzer-config-help >> >> Differential Revision: https://reviews.llvm.org/D53296 >> >> Added: >> cfe/trunk/test/Analysis/analyzer-list-configs.c >> Modified: >> cfe/trunk/include/clang/Driver/CC1Options.td >> cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h >> cfe/trunk/include/clang/StaticAnalyzer/Frontend/FrontendActions.h >> cfe/trunk/lib/Frontend/CompilerInvocation.cpp >> cfe/trunk/lib/FrontendTool/ExecuteCompilerInvocation.cpp >> cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp >> >> Modified: cfe/trunk/include/clang/Driver/CC1Options.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=345989&r1=345988&r2=345989&view=diff >> >> == >> --- cfe/trunk/include/clang/Driver/CC1Options.td (original) >> +++ cfe/trunk/include/clang/Driver/CC1Options.td Fri Nov 2 08:59:37 2018 >> @@ -129,6 +129,9 @@ def analyzer_disable_all_checks : Flag<[ >> def analyzer_checker_help : Flag<["-"], "analyzer-checker-help">, >>HelpText<"Display the list of analyzer checkers that are available">; >> >> +def analyzer_config_help : Flag<["-"], "analyzer-config-help">, >> + HelpText<"Display the list of -analyzer-config options">; >> + >> def analyzer_list_enabled_checkers : Flag<["-"], >> "analyzer-list-enabled-checkers">, >>HelpText<"Display the list of enabled analyzer checkers">; >> >> >> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h?rev=345989&r1=345988&r2=345989&view=diff >> >> == >> --- cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h >> (original) >> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h Fri >> Nov 2 08:59:37 2018 >> @@ -175,6 +175,7 @@ public: >> >>unsigned ShowCheckerHelp : 1; >>unsigned ShowEnabledCheckerList : 1; >> + unsigned ShowConfigOptionsList : 1; >>unsigned AnalyzeAll : 1; >>unsigned AnalyzerDisplayProgress : 1; >>unsigned AnalyzeNestedBlocks : 1; >> >> Modified: >> cfe/trunk/include/clang/StaticAnalyzer/Frontend/FrontendActions.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Frontend/FrontendActions.h?rev=345989&r1=345988&r2=345989&view=diff >> >> == >> --- cfe/trunk/include/clang/StaticAnalyzer/Frontend/FrontendActions.h >> (original) >> +++ cfe/trunk/include/clang/StaticAnalyzer/Frontend/FrontendActions.h Fri >> Nov 2 08:59:37 2018 >> @@ -55,6 +55,7 @@ private: >> void printCheckerHelp(raw_ostream &OS, ArrayRef plugins); >> void printEnabledCheckerList(raw_ostream &OS, ArrayRef >> plugins, >> const AnalyzerOptions &opts); >> +void printAnalyzerConfigList(raw_ostream &OS); >> >> } // end GR namespace >> >> >> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=345989&r1=345988&r2=345989&view=diff >> >> == >> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original) >> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Nov 2 08:59:37 2018 >> @@ -279,6 +279,7 @@ static bool ParseAnalyzerArgs(AnalyzerOp >>} >> >>Opts.ShowCheckerHelp = Args.hasArg(OPT_analyzer_checker_help); >> + Opts.ShowConfigOptionsList = Args.hasArg(OPT_analyzer_config_help); >>Opts.ShowEnabledCheckerList = >> Args.hasArg(OPT_analyzer_list_enabled_checkers); >>Opts.DisableAllChecks = Args.hasArg(OPT_analyzer_disable_all_checks); >> >> >> Modified: cfe/trunk/lib/FrontendTool/ExecuteCompilerInvocation.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/FrontendTool/ExecuteCompilerInvocation.cpp?rev=345989&r1=345988
[PATCH] D53984: [mips] Fix broken MSA test
This revision was automatically updated to reflect the committed changes. Closed by commit rL346124: [mips][msa] Fix broken test (authored by abeserminji, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D53984?vs=172152&id=172554#toc Repository: rL LLVM https://reviews.llvm.org/D53984 Files: cfe/trunk/test/CodeGen/builtins-mips-msa-error.c Index: cfe/trunk/test/CodeGen/builtins-mips-msa-error.c === --- cfe/trunk/test/CodeGen/builtins-mips-msa-error.c +++ cfe/trunk/test/CodeGen/builtins-mips-msa-error.c @@ -1,18 +1,22 @@ // REQUIRES: mips-registered-target -// RUN: not %clang_cc1 -triple mips-unknown-linux-gnu -fsyntax-only %s \ +// RUN: %clang_cc1 -triple mips-unknown-linux-gnu -fsyntax-only %s \ // RUN:-target-feature +msa -target-feature +fp64 \ -// RUN:-mfloat-abi hard -o - 2>&1 | FileCheck %s +// RUN:-verify -mfloat-abi hard -o - 2>&1 #include void test(void) { v16i8 v16i8_a = (v16i8) {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; + v16i8 v16i8_b = (v16i8) {16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}; v16i8 v16i8_r; v8i16 v8i16_a = (v8i16) {0, 1, 2, 3, 4, 5, 6, 7}; + v8i16 v8i16_b = (v8i16) {8, 9, 10, 11, 12, 13, 14, 15}; v8i16 v8i16_r; v4i32 v4i32_a = (v4i32) {0, 1, 2, 3}; + v4i32 v4i32_b = (v4i32) {4, 5, 6, 7}; v4i32 v4i32_r; v2i64 v2i64_a = (v2i64) {0, 1}; + v2i64 v2i64_b = (v2i64) {3, 4}; v2i64 v2i64_r; v16u8 v16u8_a = (v16u8) {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; @@ -24,388 +28,385 @@ v2u64 v2u64_a = (v2u64) {0, 1}; v2u64 v2u64_r; - int int_r; long long ll_r; - v16u8_r = __msa_addvi_b(v16u8_a, 32); // expected-error {{argument should be a value from 0 to 31}} - v8u16_r = __msa_addvi_h(v8u16_a, 32); // expected-error {{argument should be a value from 0 to 31}} - v4u32_r = __msa_addvi_w(v4u32_a, 32); // expected-error {{argument should be a value from 0 to 31}} - v2u64_r = __msa_addvi_d(v2u64_a, 32); // expected-error {{argument should be a value from 0 to 31}} - - v16i8_r = __msa_andi_b(v16i8_a, 256); // CHECK: warning: argument should be a value from 0 to 255}} - v8i16_r = __msa_andi_b(v8i16_a, 256); // CHECK: warning: argument should be a value from 0 to 255}} - v4i32_r = __msa_andi_b(v4i32_a, 256); // CHECK: warning: argument should be a value from 0 to 255}} - v2i64_r = __msa_andi_b(v2i64_a, 256); // CHECK: warning: argument should be a value from 0 to 255}} - - v16i8_r = __msa_bclri_b(v16i8_a, 8); // expected-error {{argument should be a value from 0 to 7}} - v8i16_r = __msa_bclri_h(v8i16_a, 16); // expected-error {{argument should be a value from 0 to 15}} - v4i32_r = __msa_bclri_w(v4i32_a, 33); // expected-error {{argument should be a value from 0 to 31}} - v2i64_r = __msa_bclri_d(v2i64_a, 64); // expected-error {{argument should be a value from 0 to 63}} - - v16i8_r = __msa_binsli_b(v16i8_r, v16i8_a, 8); // expected-error {{argument should be a value from 0 to 7}} - v8i16_r = __msa_binsli_h(v8i16_r, v8i16_a, 16);// expected-error {{argument should be a value from 0 to 15}} - v4i32_r = __msa_binsli_w(v4i32_r, v4i32_a, 32);// expected-error {{argument should be a value from 0 to 31}} - v2i64_r = __msa_binsli_d(v2i64_r, v2i64_a, 64);// expected-error {{argument should be a value from 0 to 63}} - - v16i8_r = __msa_binsri_b(v16i8_r, v16i8_a, 8); // expected-error {{argument should be a value from 0 to 7}} - v8i16_r = __msa_binsri_h(v8i16_r, v8i16_a, 16);// expected-error {{argument should be a value from 0 to 15}} - v4i32_r = __msa_binsri_w(v4i32_r, v4i32_a, 32);// expected-error {{argument should be a value from 0 to 31}} - v2i64_r = __msa_binsri_d(v2i64_r, v2i64_a, 64);// expected-error {{argument should be a value from 0 to 63}} - - v16i8_r = __msa_bmnzi_b(v16i8_r, v16i8_a, 256);// expected-error {{argument should be a value from 0 to 255}} - - v16i8_r = __msa_bmzi_b(v16i8_r, v16i8_a, 256); // expected-error {{argument should be a value from 0 to 255}} - - v16i8_r = __msa_bnegi_b(v16i8_a, 8); // expected-error {{argument should be a value from 0 to 7}} - v8i16_r = __msa_bnegi_h(v8i16_a, 16); // expected-error {{argument should be a value from 0 to 15}} - v4i32_r = __msa_bnegi_w(v4i32_a, 32); // expected-error {{argument should be a value from 0 to 31}} - v2i64_r = __msa_bnegi_d(v2i64_a, 64); // expected-error {{argument should be a value from 0 to 63}} - - v16i8_r = __msa_bseli_b(v16i8_r, v16i8_a, 256);// expected-error {{argument should be a value from 0 to 255}} - - v16i8_r = __msa_bseti_b(v16i8_a, 8); // expected-error {{argumen
[PATCH] D54077: [clangd] Implemented DraftFileSystem
ilya-biryukov added subscribers: simark, klimek. ilya-biryukov added a comment. Thanks for the patch! I believe many people I talked to want this behavior (myself included). Some people like what we do now more. It feels like it depends on the workflow: for people who auto-save *all* files before build (some editors do it automatically?) the new behavior is the right one, for people who only save current file and rerun build on the console the old behavior is the correct one. It all boils down to the argument "I want to see same errors that running the compiler would produce". @klimek, @arphaman, @simark, WDYT? Comment at: clangd/FS.h:82 + +if (auto D = DS.getDraft(Path.str())) { + return std::unique_ptr( This assumes the `Path` is absolute and `vfs::FileSystem` can be called with non-absolute paths too. One way to make it work with relative paths is to create an `InMemoryFileSystem` with the headers (it handles the absolute paths conversions) and create an `OverlayFileSystem` on top of it and the `RealFileSystem`. This would also make more complicated things work, e.g. getting the same files when traversing parent directories, etc. Could we try this approach? WDYT? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
gchatelet marked 8 inline comments as done. gchatelet added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } JonasToth wrote: > lebedev.ri wrote: > > courbet wrote: > > > What about providing a fix for this one : > > > `while (i != 0) {` > > Is this //actually// a narrowing conversion as per the CPPCG? > > Conversion to bool is a special case, it's not a truncation, but a `x != 0`. > > I'd think cases like these where bool was pretty clearly meant (`if()`, > > `while()`, `for()`, ???) should be exempt. > No it is not an narrowing conversion the CPPCG are concerned: > https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions > > Short: > ``` > ES.46: Avoid lossy (narrowing, truncating) arithmetic conversions > > Enforcement > > A good analyzer can detect all narrowing conversions. However, flagging all > narrowing conversions will lead to a lot of false positives. Suggestions: > > - flag all floating-point to integer conversions (maybe only float->char > and double->int. Here be dragons! we need data) > - flag all long->char (I suspect int->char is very common. Here be > dragons! we need data) > - consider narrowing conversions for function arguments especially suspect > ``` > > We do have a readability-check that is concerned about int -> bool > conversions. Because of that I think we could safely ignore the `int->bool` > conversion. > But `float -> bool` is still suspicious in my opinion. It has the same > semantics, but comparing `float` on equality is dangerous and one could argue > it shall be enforced through point 1 (all float-to-int conversions). @JonasToth Do you have the name of the readability check so I can document the behavior? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
hokein updated this revision to Diff 172560. hokein marked an inline comment as done. hokein added a comment. Update comment. Repository: rC Clang https://reviews.llvm.org/D54092 Files: lib/Tooling/AllTUsExecution.cpp Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,12 @@ } // namespace +static llvm::cl::opt Filter( +"filter", +llvm::cl::desc("Only process files that match this filter. " + "This flag only applies to all-TUs."), +llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +116,10 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + @@ -147,7 +156,8 @@ static llvm::cl::opt ExecutorConcurrency( "execute-concurrency", llvm::cl::desc("The number of threads used to process all files in " - "parallel. Set to 0 for hardware concurrency."), + "parallel. Set to 0 for hardware concurrency. " + "This flag only applies to all-TUs."), llvm::cl::init(0)); class AllTUsToolExecutorPlugin : public ToolExecutorPlugin { Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,12 @@ } // namespace +static llvm::cl::opt Filter( +"filter", +llvm::cl::desc("Only process files that match this filter. " + "This flag only applies to all-TUs."), +llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +116,10 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + @@ -147,7 +156,8 @@ static llvm::cl::opt ExecutorConcurrency( "execute-concurrency", llvm::cl::desc("The number of threads used to process all files in " - "parallel. Set to 0 for hardware concurrency."), + "parallel. Set to 0 for hardware concurrency. " + "This flag only applies to all-TUs."), llvm::cl::init(0)); class AllTUsToolExecutorPlugin : public ToolExecutorPlugin { ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
hokein added inline comments. Comment at: lib/Tooling/AllTUsExecution.cpp:120 for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; ioeric wrote: > > `Filter.getNumOccurrences() != 0 ` > Would this work if `Filter` is set grammatically? Although it's not exposed > now, it might make more sense to check `Filter != ".*"`. Yes, getNumOccurrences returns non-zero only when the flag is set by the command line. ``` clang-tool -executor=all-TUs . # => getNumOccurrences() == 0 clang-tool -executor=all-TUs -filter="xx" . # > getNumOccurrences() != 0 ``` Repository: rC Clang https://reviews.llvm.org/D54092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded
Szelethus marked 6 inline comments as done. Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:677 +/// need to expanded further when it is nested inside another macro. +class MacroArgMap : public std::map { +public: NoQ wrote: > Szelethus wrote: > > Szelethus wrote: > > > george.karpenkov wrote: > > > > Please don't do this, inheritance from standard containers is a bad > > > > thing (tm) > > > Actually, I disagree with you on this one. For ease of use, these > > > constructs are used all over the place in the LLVM codebase, and since I > > > never do anything inheritance related, this shouldn't hurt much. > > > > > > https://clang.llvm.org/doxygen/classclang_1_1ento_1_1PathPieces.html > > > http://llvm.org/doxygen/classllvm_1_1PredicateBitsetImpl.html > > > http://llvm.org/doxygen/classllvm_1_1HexagonBlockRanges_1_1RangeList.html > > > > > > These are just a few. > > > > > > Do you insist? > > I mean, polymorphism, not inheritance. > Dunno why exactly, but George really hates these :) > > To me it's a reasonable thing to use in a tiny utility class - as opposed to > re-implementing all vector methods in a composition every time you need just > one extra method. > > It should also be possible to avoid this by sacrificing object-oriented-ness > by turning the newly added method into a standalone function, i.e.: > ``` > using MacroArgMap = std::map; > void expandFromPrevMacro(MacroArgMap &This, const MacroArgMap &Super); > ``` > > Which also seems almost free. I personally very much prefer the current state :/ It is locally defined on a bottom of a file, I think I'll commit as is. Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:965-966 + + It = CurrExpArgTokens.insert( + It, SuperExpArgTokens.begin(), SuperExpArgTokens.end()); + std::advance(It, SuperExpArgTokens.size()); NoQ wrote: > I think it's the first time in my life when i see a loop that (correctly) > mutates the container :) Yay ^-^ https://reviews.llvm.org/D52795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
ioeric added inline comments. Comment at: lib/Tooling/AllTUsExecution.cpp:120 for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; hokein wrote: > ioeric wrote: > > > `Filter.getNumOccurrences() != 0 ` > > Would this work if `Filter` is set grammatically? Although it's not exposed > > now, it might make more sense to check `Filter != ".*"`. > Yes, getNumOccurrences returns non-zero only when the flag is set by the > command line. > > ``` > clang-tool -executor=all-TUs . # => getNumOccurrences() == 0 > clang-tool -executor=all-TUs -filter="xx" . # > getNumOccurrences() != 0 > ``` Sorry, I meant "programmatically"... something like `Filter.setInitialValue("...")`. Repository: rC Clang https://reviews.llvm.org/D54092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54097: Save memory in ASTReader by using DenseMap instead of vector
yamaguchi created this revision. yamaguchi added a reviewer: rsmith. ReadASTBlock was allocating extra memory by resizing vector for all record decls, and half of them stayed nullptr without being loaded. For us this part was crucial and this patch had large amount of memory improvement. About clang performance regression, I run whole clang test 5 times with and without this patch and took average: Without patch: 239.434003 With patch : 238.862 So I think this doesn't cause cpu time regression. https://reviews.llvm.org/D54097 Files: clang/include/clang/Serialization/ASTReader.h clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTReaderDecl.cpp Index: clang/lib/Serialization/ASTReaderDecl.cpp === --- clang/lib/Serialization/ASTReaderDecl.cpp +++ clang/lib/Serialization/ASTReaderDecl.cpp @@ -2748,8 +2748,9 @@ /// so that future GetDecl calls will return this declaration rather /// than trying to load a new declaration. inline void ASTReader::LoadedDecl(unsigned Index, Decl *D) { - assert(!DeclsLoaded[Index] && "Decl loaded twice?"); - DeclsLoaded[Index] = D; + assert((DeclsLoaded.find(Index) == DeclsLoaded.end()) && + "Decl loaded twice?"); + DeclsLoaded.insert({Index, D}); } /// Determine whether the consumer will be interested in seeing Index: clang/lib/Serialization/ASTReader.cpp === --- clang/lib/Serialization/ASTReader.cpp +++ clang/lib/Serialization/ASTReader.cpp @@ -2834,7 +2834,7 @@ std::make_pair(LocalBaseTypeIndex, F.BaseTypeIndex - LocalBaseTypeIndex)); -TypesLoaded.resize(TypesLoaded.size() + F.LocalNumTypes); +NumTypes += F.LocalNumTypes; } break; } @@ -2864,7 +2864,7 @@ // module. F.GlobalToLocalDeclIDs[&F] = LocalBaseDeclID; -DeclsLoaded.resize(DeclsLoaded.size() + F.LocalNumDecls); +NumDecls += F.LocalNumDecls; } break; } @@ -3425,7 +3425,7 @@ std::make_pair(LocalBaseMacroID, F.BaseMacroID - LocalBaseMacroID)); -MacrosLoaded.resize(MacrosLoaded.size() + F.LocalNumMacros); +NumMacros += F.LocalNumMacros; } break; } @@ -7000,19 +7000,19 @@ } Index -= NUM_PREDEF_TYPE_IDS; - assert(Index < TypesLoaded.size() && "Type index out-of-range"); - if (TypesLoaded[Index].isNull()) { -TypesLoaded[Index] = readTypeRecord(Index); -if (TypesLoaded[Index].isNull()) + assert(Index < NumTypes && "Type index out-of-range"); + if (TypesLoaded.find(Index) == TypesLoaded.end()) { +QualType QualTy = readTypeRecord(Index); +TypesLoaded.insert({Index, QualTy}); +if (TypesLoaded.find(Index) == TypesLoaded.end()) return QualType(); -TypesLoaded[Index]->setFromAST(); +QualTy->setFromAST(); if (DeserializationListener) - DeserializationListener->TypeRead(TypeIdx::fromTypeID(ID), -TypesLoaded[Index]); + DeserializationListener->TypeRead(TypeIdx::fromTypeID(ID), QualTy); } - return TypesLoaded[Index].withFastQualifiers(FastQuals); + return TypesLoaded.find(Index)->second.withFastQualifiers(FastQuals); } QualType ASTReader::getLocalType(ModuleFile &F, unsigned LocalID) { @@ -7241,13 +7241,14 @@ unsigned Index = ID - NUM_PREDEF_DECL_IDS; - if (Index > DeclsLoaded.size()) { + if (Index > NumDecls) { Error("declaration ID out-of-range for AST file"); return SourceLocation(); } - if (Decl *D = DeclsLoaded[Index]) -return D->getLocation(); + auto FindRes = DeclsLoaded.find(Index); + if (FindRes != DeclsLoaded.end()) +return FindRes->second->getLocation(); SourceLocation Loc; DeclCursorForID(ID, Loc); @@ -7326,34 +7327,38 @@ unsigned Index = ID - NUM_PREDEF_DECL_IDS; - if (Index >= DeclsLoaded.size()) { + if (Index >= NumDecls) { assert(0 && "declaration ID out-of-range for AST file"); Error("declaration ID out-of-range for AST file"); return nullptr; } - return DeclsLoaded[Index]; + auto FindRes = DeclsLoaded.find(Index); + if (FindRes == DeclsLoaded.end()) +return nullptr; + + return FindRes->second; } Decl *ASTReader::GetDecl(DeclID ID) { if (ID < NUM_PREDEF_DECL_IDS) return GetExistingDecl(ID); unsigned Index = ID - NUM_PREDEF_DECL_IDS; - if (Index >= DeclsLoaded.size()) { + if (Index >= NumDecls) { assert(0 && "declaration ID out-of-range for AST file"); Error("declaration ID out-of-range for AST file"); return nullptr; } - if (!DeclsLoaded[Index]) { + if (DeclsLoaded.find(Index) == DeclsLoaded.end()) { ReadDeclRecord(ID); if (DeserializationListener) - DeserializationListener->DeclRead(ID, DeclsLoaded[Index]); + DeserializationListener->DeclRead(ID, DeclsLoaded.find(Index)->second); } - ret
[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition
ebevhan added a comment. In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote: > >> 2. The question is easily answered by pointing at the language spec. The > >> language does not say that the operands are promoted to a common type; it > >> says the result is determined numerically from the true numeric values of > >> the operands. > > > > I guess. I just see that as a behavioral specification and not necessarily > > an implementation detail. It's perfectly acceptable to implement it as > > promotion to a common type (obviously, as that's what we are going to do), > > and I don't really see this as the spec putting any kind of requirement on > > how the implementation should be done. > > Of course it's a behavioral specification. All I'm saying is that the > implementation detail of how we perform these operations isn't really > reasonable or appropriate to express in the AST. I know it's a bit fuzzy > because of some of the things we do with e.g. opaque values and so on, but > the AST is not meant to be a completely implementation-defined IR; it tries > to capture the formal semantics of the program including "official" > conversions and so on. Integer addition is specified as converting its > arguments to a common type and then performing a homogenous operation in that > type. Fixed-point addition is not; it's specified as doing a heterogenous > operation on the original (well, rvalue-converted) operand types. Okay, those are good points. I guess I might have been a bit too focused on reusing the behavior of the AST to fit the implementation, but that doesn't seem to be the intention with it. > > >> It might just be easier to store the full-precision info in BO directly. >> BO might be too common to warrant the size increase, though. >> FixedPointSemantics can probably be optimized to only take 32 bits. > > What you can definitely do is store a bit in BO saying that there's extra > storage for the intermediate "type". Is this similar to how ExtQuals works? How would this be implemented? >>> >>> Take a look at how `DeclRefExpr` stores its various optional components. >> >> `TrailingObjects`, then. That certainly wouldn't work if >> CompoundAssignOperator is still a subclass of BinaryOperator, so it would >> have to be folded in that case. >> >> So the implementation would be with a `TrailingObjects> QualType, FixedPointSemantics>` where it can have 2 QualTypes and 1 >> FixedPointSemantics, the QualTypes being subsumed from >> CompoundAssignOperator. >> >> Probably still quite a hefty amount of code that would have to be updated to >> make this change. > > Not out of line with other features that significantly break with what's > expressible. But the easy alternative to storing the intermediate "type" in > the AST is to just provide a global function that can compute it on demand. Yeah, it might just be simpler to go the route of not storing the computation semantic in the AST, at least for now. That's fairly similar to the solution in the patch, so I feel a bit silly for just going around in a circle. To make that work I think the patch needs some changes, though. There should be a function in FixedPointSemantics to find the common full-precision semantic between two semantics, and the `getFixedPointSemantics` in ASTContext should be amended to take integer types (or another method should be provided for this, but that one already exists). I think that the `FixedPointConversions` method should also be embedded into the rest of `UsualArithmeticConversions` as there shouldn't be any need to have it separate. You still want to do the rvalue conversion and other promotions, and the rules for fixed-point still fit into the arithmetic conversions, even in the spec. `EmitFixedPointConversion` should be changed to take FixedPointSemantics rather than QualType. This has a couple of downsides: - It can no longer handle floating point conversions. They would have to be handled in EmitScalarConversion. - Conversion from integer is fine, but conversion to integer cannot be generalized away with the fixed-point semantics as they are currently, as that kind of conversion must round towards zero. This requires a rounding step for negative numbers before downscaling, which the current code does not do. Is there a better way of generalizing this? All `EmitFixedPointAdd` should have to do is get the semantics of the operands and result type, get their full-precision semantic, call `EmitFixedPointConversion` on both operands, do the addition, and call it again to convert back to the result value. Move as much of the conversions as possible out of the function. Does all this sound reasonable? Repository: rC Clang https://reviews.llvm.org/D53738 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54097: Save memory in ASTReader by using DenseMap instead of vector
yamaguchi added a comment. FYI, raw benchmarking data: https://gist.github.com/yamaguchi1024/4f28406f51b413129cc762365fdd76c8 https://gist.github.com/yamaguchi1024/318b30db03adb10b7b230b706012a14c https://reviews.llvm.org/D54097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } gchatelet wrote: > JonasToth wrote: > > lebedev.ri wrote: > > > courbet wrote: > > > > What about providing a fix for this one : > > > > `while (i != 0) {` > > > Is this //actually// a narrowing conversion as per the CPPCG? > > > Conversion to bool is a special case, it's not a truncation, but a `x != > > > 0`. > > > I'd think cases like these where bool was pretty clearly meant (`if()`, > > > `while()`, `for()`, ???) should be exempt. > > No it is not an narrowing conversion the CPPCG are concerned: > > https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions > > > > Short: > > ``` > > ES.46: Avoid lossy (narrowing, truncating) arithmetic conversions > > > > Enforcement > > > > A good analyzer can detect all narrowing conversions. However, flagging all > > narrowing conversions will lead to a lot of false positives. Suggestions: > > > > - flag all floating-point to integer conversions (maybe only > > float->char and double->int. Here be dragons! we need data) > > - flag all long->char (I suspect int->char is very common. Here be > > dragons! we need data) > > - consider narrowing conversions for function arguments especially > > suspect > > ``` > > > > We do have a readability-check that is concerned about int -> bool > > conversions. Because of that I think we could safely ignore the `int->bool` > > conversion. > > But `float -> bool` is still suspicious in my opinion. It has the same > > semantics, but comparing `float` on equality is dangerous and one could > > argue it shall be enforced through point 1 (all float-to-int conversions). > @JonasToth Do you have the name of the readability check so I can document > the behavior? Should be https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54077: [clangd] Implemented DraftFileSystem
klimek added a comment. In https://reviews.llvm.org/D54077#1287040, @ilya-biryukov wrote: > Thanks for the patch! I believe many people I talked to want this behavior > (myself included). > Some people like what we do now more. It feels like it depends on the > workflow: for people who auto-save *all* files before build (some editors do > it automatically?) the new behavior is the right one, for people who only > save current file and rerun build on the console the old behavior is the > correct one. > It all boils down to the argument "I want to see same errors that running > the compiler would produce". > > @klimek, @arphaman, @simark, WDYT? I'm in yet another camp: I carefully save when I have something that is correct enough syntax, so I only want errors from with changes from the exact file I'm editing and the rest of the files in saved state. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D54077: [clangd] Implemented DraftFileSystem
Disclaimer: I'm on a train with family today, and haven't actually read the patch... So I have concerns :-) 1. There's the usual concern that the current behavior is reasonable and people like it, so adding a second reasonable behavior provides a small amount of value to the userbase as a whole (because 99%+ will use the default). If the benefit is small, the comparison to support cost may be unfavorable. 2. This needs to invalidate preambles more often, which brings both performance and complexity questions. E.g. we will at some point invalidate preambles and regenerate diagnostics based on file writes. After this change, we'll be obligated to do so for edits too. (Remember, LSP has no concept of "the foreground file"). Invalidating a preamble is expensive, and edits come rapidly and may invalidate multiple TUs. The current behavior seems more compatible with being both simple and fast. So mostly I'd like to be convinced that this is important (or that it's simple, but that seems unlikely at first glance). Cheers, Sam On Mon, Nov 5, 2018, 11:32 Ilya Biryukov via Phabricator < revi...@reviews.llvm.org wrote: > ilya-biryukov added subscribers: simark, klimek. > ilya-biryukov added a comment. > > Thanks for the patch! I believe many people I talked to want this behavior > (myself included). > Some people like what we do now more. It feels like it depends on the > workflow: for people who auto-save *all* files before build (some editors > do it automatically?) the new behavior is the right one, for people who > only save current file and rerun build on the console the old behavior is > the correct one. > It all boils down to the argument "I want to see same errors that running > the compiler would produce". > > @klimek, @arphaman, @simark, WDYT? > > > > > Comment at: clangd/FS.h:82 > + > +if (auto D = DS.getDraft(Path.str())) { > + return std::unique_ptr( > > This assumes the `Path` is absolute and `vfs::FileSystem` can be called > with non-absolute paths too. > One way to make it work with relative paths is to create an > `InMemoryFileSystem` with the headers (it handles the absolute paths > conversions) and create an `OverlayFileSystem` on top of it and the > `RealFileSystem`. > > This would also make more complicated things work, e.g. getting the same > files when traversing parent directories, etc. > > Could we try this approach? WDYT? > > > > Repository: > rCTE Clang Tools Extra > > https://reviews.llvm.org/D54077 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54077: [clangd] Implemented DraftFileSystem
sammccall added subscribers: ilya-biryukov, LutsenkoDanil. sammccall added a comment. Disclaimer: I'm on a train with family today, and haven't actually read the patch... So I have concerns :-) 1. There's the usual concern that the current behavior is reasonable and people like it, so adding a second reasonable behavior provides a small amount of value to the userbase as a whole (because 99%+ will use the default). If the benefit is small, the comparison to support cost may be unfavorable. 2. This needs to invalidate preambles more often, which brings both performance and complexity questions. E.g. we will at some point invalidate preambles and regenerate diagnostics based on file writes. After this change, we'll be obligated to do so for edits too. (Remember, LSP has no concept of "the foreground file"). Invalidating a preamble is expensive, and edits come rapidly and may invalidate multiple TUs. The current behavior seems more compatible with being both simple and fast. So mostly I'd like to be convinced that this is important (or that it's simple, but that seems unlikely at first glance). Cheers, Sam Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64
joerg added a comment. Nothing changed. I don't see how catering to the broken libstdc++ self-configuration helps. So no, I still object to this change. Repository: rL LLVM https://reviews.llvm.org/D34018 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346130 - Reapply "Fix regression in behavior of clang -x c++-header -fmodule-name=XXX"
Author: d0k Date: Mon Nov 5 04:46:02 2018 New Revision: 346130 URL: http://llvm.org/viewvc/llvm-project?rev=346130&view=rev Log: Reapply "Fix regression in behavior of clang -x c++-header -fmodule-name=XXX" This reverts commit r345963. We have a path forward now. Original commit message: The driver accidentally stopped passing the input filenames on to -cc1 in this mode due to confusion over what action was being requested. This change also fixes a couple of crashes I encountered when passing multiple files to such a -cc1 invocation. Added: cfe/trunk/test/Modules/strict-decluse-headers.cpp Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp cfe/trunk/lib/Frontend/CompilerInstance.cpp cfe/trunk/lib/Lex/ModuleMap.cpp cfe/trunk/test/Driver/header-module.cpp Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=346130&r1=346129&r2=346130&view=diff == --- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original) +++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Mon Nov 5 04:46:02 2018 @@ -3250,18 +3250,15 @@ void Clang::ConstructJob(Compilation &C, bool IsCuda = JA.isOffloading(Action::OFK_Cuda); bool IsHIP = JA.isOffloading(Action::OFK_HIP); bool IsOpenMPDevice = JA.isDeviceOffloading(Action::OFK_OpenMP); - bool IsModulePrecompile = - isa(JA) && JA.getType() == types::TY_ModuleFile; bool IsHeaderModulePrecompile = isa(JA); // A header module compilation doesn't have a main input file, so invent a // fake one as a placeholder. - // FIXME: Pick the language based on the header file language. const char *ModuleName = [&]{ auto *ModuleNameArg = Args.getLastArg(options::OPT_fmodule_name_EQ); return ModuleNameArg ? ModuleNameArg->getValue() : ""; }(); - InputInfo HeaderModuleInput(types::TY_CXXModule, ModuleName, ModuleName); + InputInfo HeaderModuleInput(Inputs[0].getType(), ModuleName, ModuleName); const InputInfo &Input = IsHeaderModulePrecompile ? HeaderModuleInput : Inputs[0]; @@ -3272,10 +3269,9 @@ void Clang::ConstructJob(Compilation &C, for (const InputInfo &I : Inputs) { if (&I == &Input) { // This is the primary input. -} else if (IsModulePrecompile && +} else if (IsHeaderModulePrecompile && types::getPrecompiledType(I.getType()) == types::TY_PCH) { - types::ID Expected = - types::lookupHeaderTypeForSourceType(Inputs[0].getType()); + types::ID Expected = HeaderModuleInput.getType(); if (I.getType() != Expected) { D.Diag(diag::err_drv_module_header_wrong_kind) << I.getFilename() << types::getTypeName(I.getType()) Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=346130&r1=346129&r2=346130&view=diff == --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original) +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Mon Nov 5 04:46:02 2018 @@ -372,6 +372,9 @@ static void InitializeFileRemapping(Diag void CompilerInstance::createPreprocessor(TranslationUnitKind TUKind) { const PreprocessorOptions &PPOpts = getPreprocessorOpts(); + // The module manager holds a reference to the old preprocessor (if any). + ModuleManager.reset(); + // Create a PTH manager if we are using some form of a token cache. PTHManager *PTHMgr = nullptr; if (!PPOpts.TokenCache.empty()) Modified: cfe/trunk/lib/Lex/ModuleMap.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/ModuleMap.cpp?rev=346130&r1=346129&r2=346130&view=diff == --- cfe/trunk/lib/Lex/ModuleMap.cpp (original) +++ cfe/trunk/lib/Lex/ModuleMap.cpp Mon Nov 5 04:46:02 2018 @@ -523,7 +523,7 @@ void ModuleMap::diagnoseHeaderInclusion( // At this point, only non-modular includes remain. - if (LangOpts.ModulesStrictDeclUse) { + if (RequestingModule && LangOpts.ModulesStrictDeclUse) { Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module) << RequestingModule->getTopLevelModule()->Name << Filename; } else if (RequestingModule && RequestingModuleIsModuleInterface && Modified: cfe/trunk/test/Driver/header-module.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/header-module.cpp?rev=346130&r1=346129&r2=346130&view=diff == --- cfe/trunk/test/Driver/header-module.cpp (original) +++ cfe/trunk/test/Driver/header-module.cpp Mon Nov 5 04:46:02 2018 @@ -7,7 +7,18 @@ // CHECK-PRECOMPILE-SAME: -fno-implicit-modules // CHECK-PRECOMPILE-SAME: -fmodule-name=foobar // CHECK-PRECOMPILE-SAME: -o {{.*}}.pcm -// CHECK-PRECOMPILE-SAME: -x c++ +// CHECK-PRECOMPILE-SAME: -x c++-header //
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
hokein added inline comments. Comment at: lib/Tooling/AllTUsExecution.cpp:120 for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; ioeric wrote: > hokein wrote: > > ioeric wrote: > > > > `Filter.getNumOccurrences() != 0 ` > > > Would this work if `Filter` is set grammatically? Although it's not > > > exposed now, it might make more sense to check `Filter != ".*"`. > > Yes, getNumOccurrences returns non-zero only when the flag is set by the > > command line. > > > > ``` > > clang-tool -executor=all-TUs . # => getNumOccurrences() == 0 > > clang-tool -executor=all-TUs -filter="xx" . # > getNumOccurrences() != 0 > > ``` > Sorry, I meant "programmatically"... something like > `Filter.setInitialValue("...")`. hmm, it works the same as `llvm::cl::init` (`getNumOccurrences` returns 0). I removed this check (just use `if (!Filter.match)`), the reason I added here is to avoid the cost of regex match, but it seems pre-optimization. Repository: rC Clang https://reviews.llvm.org/D54092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
hokein updated this revision to Diff 172571. hokein added a comment. Remove the default value check. Repository: rC Clang https://reviews.llvm.org/D54092 Files: lib/Tooling/AllTUsExecution.cpp Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,12 @@ } // namespace +static llvm::cl::opt Filter( +"filter", +llvm::cl::desc("Only process files that match this filter. " + "This flag only applies to all-TUs."), +llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +116,10 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (!RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + @@ -147,7 +156,8 @@ static llvm::cl::opt ExecutorConcurrency( "execute-concurrency", llvm::cl::desc("The number of threads used to process all files in " - "parallel. Set to 0 for hardware concurrency."), + "parallel. Set to 0 for hardware concurrency. " + "This flag only applies to all-TUs."), llvm::cl::init(0)); class AllTUsToolExecutorPlugin : public ToolExecutorPlugin { Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,12 @@ } // namespace +static llvm::cl::opt Filter( +"filter", +llvm::cl::desc("Only process files that match this filter. " + "This flag only applies to all-TUs."), +llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +116,10 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (!RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + @@ -147,7 +156,8 @@ static llvm::cl::opt ExecutorConcurrency( "execute-concurrency", llvm::cl::desc("The number of threads used to process all files in " - "parallel. Set to 0 for hardware concurrency."), + "parallel. Set to 0 for hardware concurrency. " + "This flag only applies to all-TUs."), llvm::cl::init(0)); class AllTUsToolExecutorPlugin : public ToolExecutorPlugin { ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54077: [clangd] Implemented DraftFileSystem
ilya-biryukov added a comment. > There's the usual concern that the current behavior is reasonable and people > like it. I think it would be reasonable to say that a large portion of C++ users are used to the behavior that this patch introduces. Specifically, all IDEs (Clion, Eclipse, Visual Studio for certain, not sure about XCode) will see the contents of the headers that the user typed and not the other way around. It does not cause any inconsistencies there, the IDE will save all your open files when you hit "build". That also means reading files from the filesystem gives non-useful errors in that case, because the build will actually see the files from the editors. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: lib/Tooling/AllTUsExecution.cpp:120 for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; hokein wrote: > ioeric wrote: > > hokein wrote: > > > ioeric wrote: > > > > > `Filter.getNumOccurrences() != 0 ` > > > > Would this work if `Filter` is set grammatically? Although it's not > > > > exposed now, it might make more sense to check `Filter != ".*"`. > > > Yes, getNumOccurrences returns non-zero only when the flag is set by the > > > command line. > > > > > > ``` > > > clang-tool -executor=all-TUs . # => getNumOccurrences() == 0 > > > clang-tool -executor=all-TUs -filter="xx" . # > getNumOccurrences() != 0 > > > ``` > > Sorry, I meant "programmatically"... something like > > `Filter.setInitialValue("...")`. > hmm, it works the same as `llvm::cl::init` (`getNumOccurrences` returns 0). > > I removed this check (just use `if (!Filter.match)`), the reason I added here > is to avoid the cost of regex match, but it seems pre-optimization. sounds good. Repository: rC Clang https://reviews.llvm.org/D54092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
ioeric added a comment. maybe add a test? Repository: rC Clang https://reviews.llvm.org/D54092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
hokein updated this revision to Diff 172574. hokein added a comment. Add test. Repository: rC Clang https://reviews.llvm.org/D54092 Files: include/clang/Tooling/AllTUsExecution.h lib/Tooling/AllTUsExecution.cpp unittests/Tooling/ExecutionTest.cpp Index: unittests/Tooling/ExecutionTest.cpp === --- unittests/Tooling/ExecutionTest.cpp +++ unittests/Tooling/ExecutionTest.cpp @@ -248,19 +248,22 @@ MATCHER_P(Named, Name, "") { return arg.first == Name; } TEST(AllTUsToolTest, AFewFiles) { - FixedCompilationDatabaseWithFiles Compilations(".", {"a.cc", "b.cc", "c.cc"}, - std::vector()); + FixedCompilationDatabaseWithFiles Compilations( + ".", {"a.cc", "b.cc", "c.cc", "ignore.cc"}, std::vector()); AllTUsToolExecutor Executor(Compilations, /*ThreadCount=*/0); + Filter.setValue("[a-c].cc"); Executor.mapVirtualFile("a.cc", "void x() {}"); Executor.mapVirtualFile("b.cc", "void y() {}"); Executor.mapVirtualFile("c.cc", "void z() {}"); + Executor.mapVirtualFile("ignore.cc", "void d() {}"); auto Err = Executor.execute(std::unique_ptr( new ReportResultActionFactory(Executor.getExecutionContext(; ASSERT_TRUE(!Err); EXPECT_THAT( Executor.getToolResults()->AllKVResults(), ::testing::UnorderedElementsAre(Named("x"), Named("y"), Named("z"))); + Filter.setValue(".*"); // reset to default value. } TEST(AllTUsToolTest, ManyFiles) { Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,12 @@ } // namespace +llvm::cl::opt +Filter("filter", + llvm::cl::desc("Only process files that match this filter. " + "This flag only applies to all-TUs."), + llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +116,10 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (!RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + @@ -147,7 +156,8 @@ static llvm::cl::opt ExecutorConcurrency( "execute-concurrency", llvm::cl::desc("The number of threads used to process all files in " - "parallel. Set to 0 for hardware concurrency."), + "parallel. Set to 0 for hardware concurrency. " + "This flag only applies to all-TUs."), llvm::cl::init(0)); class AllTUsToolExecutorPlugin : public ToolExecutorPlugin { Index: include/clang/Tooling/AllTUsExecution.h === --- include/clang/Tooling/AllTUsExecution.h +++ include/clang/Tooling/AllTUsExecution.h @@ -72,6 +72,8 @@ unsigned ThreadCount; }; +extern llvm::cl::opt Filter; + } // end namespace tooling } // end namespace clang Index: unittests/Tooling/ExecutionTest.cpp === --- unittests/Tooling/ExecutionTest.cpp +++ unittests/Tooling/ExecutionTest.cpp @@ -248,19 +248,22 @@ MATCHER_P(Named, Name, "") { return arg.first == Name; } TEST(AllTUsToolTest, AFewFiles) { - FixedCompilationDatabaseWithFiles Compilations(".", {"a.cc", "b.cc", "c.cc"}, - std::vector()); + FixedCompilationDatabaseWithFiles Compilations( + ".", {"a.cc", "b.cc", "c.cc", "ignore.cc"}, std::vector()); AllTUsToolExecutor Executor(Compilations, /*ThreadCount=*/0); + Filter.setValue("[a-c].cc"); Executor.mapVirtualFile("a.cc", "void x() {}"); Executor.mapVirtualFile("b.cc", "void y() {}"); Executor.mapVirtualFile("c.cc", "void z() {}"); + Executor.mapVirtualFile("ignore.cc", "void d() {}"); auto Err = Executor.execute(std::unique_ptr( new ReportResultActionFactory(Executor.getExecutionContext(; ASSERT_TRUE(!Err); EXPECT_THAT( Executor.getToolResults()->AllKVResults(), ::testing::UnorderedElementsAre(Named("x"), Named("y"), Named("z"))); + Filter.setValue(".*"); // reset to default value. } TEST(AllTUsToolTest, ManyFiles) { Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,12 @@ } // namespace +llvm::cl::opt +Filter("filter", + llvm::cl::desc("Only process files that match this filter. " + "This flag only applies to all-TUs."), + llvm::cl::init(".*")); + AllTUsToolExecut
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
gamesh411 added a comment. Hi! Thanks for your reviews, although I haven't been active for some time now. I personally do not have commit rights, so could someone else take care of it? https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution
This revision was automatically updated to reflect the committed changes. Closed by commit rC346131: [Tooling] Add "-filter" option to AllTUsExecution (authored by hokein, committed by ). Changed prior to commit: https://reviews.llvm.org/D54092?vs=172574&id=172575#toc Repository: rC Clang https://reviews.llvm.org/D54092 Files: include/clang/Tooling/AllTUsExecution.h lib/Tooling/AllTUsExecution.cpp unittests/Tooling/ExecutionTest.cpp Index: unittests/Tooling/ExecutionTest.cpp === --- unittests/Tooling/ExecutionTest.cpp +++ unittests/Tooling/ExecutionTest.cpp @@ -248,19 +248,22 @@ MATCHER_P(Named, Name, "") { return arg.first == Name; } TEST(AllTUsToolTest, AFewFiles) { - FixedCompilationDatabaseWithFiles Compilations(".", {"a.cc", "b.cc", "c.cc"}, - std::vector()); + FixedCompilationDatabaseWithFiles Compilations( + ".", {"a.cc", "b.cc", "c.cc", "ignore.cc"}, std::vector()); AllTUsToolExecutor Executor(Compilations, /*ThreadCount=*/0); + Filter.setValue("[a-c].cc"); Executor.mapVirtualFile("a.cc", "void x() {}"); Executor.mapVirtualFile("b.cc", "void y() {}"); Executor.mapVirtualFile("c.cc", "void z() {}"); + Executor.mapVirtualFile("ignore.cc", "void d() {}"); auto Err = Executor.execute(std::unique_ptr( new ReportResultActionFactory(Executor.getExecutionContext(; ASSERT_TRUE(!Err); EXPECT_THAT( Executor.getToolResults()->AllKVResults(), ::testing::UnorderedElementsAre(Named("x"), Named("y"), Named("z"))); + Filter.setValue(".*"); // reset to default value. } TEST(AllTUsToolTest, ManyFiles) { Index: include/clang/Tooling/AllTUsExecution.h === --- include/clang/Tooling/AllTUsExecution.h +++ include/clang/Tooling/AllTUsExecution.h @@ -72,6 +72,8 @@ unsigned ThreadCount; }; +extern llvm::cl::opt Filter; + } // end namespace tooling } // end namespace clang Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -53,6 +53,12 @@ } // namespace +llvm::cl::opt +Filter("filter", + llvm::cl::desc("Only process files that match this filter. " + "This flag only applies to all-TUs."), + llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +116,10 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (!RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + @@ -147,7 +156,8 @@ static llvm::cl::opt ExecutorConcurrency( "execute-concurrency", llvm::cl::desc("The number of threads used to process all files in " - "parallel. Set to 0 for hardware concurrency."), + "parallel. Set to 0 for hardware concurrency. " + "This flag only applies to all-TUs."), llvm::cl::init(0)); class AllTUsToolExecutorPlugin : public ToolExecutorPlugin { Index: unittests/Tooling/ExecutionTest.cpp === --- unittests/Tooling/ExecutionTest.cpp +++ unittests/Tooling/ExecutionTest.cpp @@ -248,19 +248,22 @@ MATCHER_P(Named, Name, "") { return arg.first == Name; } TEST(AllTUsToolTest, AFewFiles) { - FixedCompilationDatabaseWithFiles Compilations(".", {"a.cc", "b.cc", "c.cc"}, - std::vector()); + FixedCompilationDatabaseWithFiles Compilations( + ".", {"a.cc", "b.cc", "c.cc", "ignore.cc"}, std::vector()); AllTUsToolExecutor Executor(Compilations, /*ThreadCount=*/0); + Filter.setValue("[a-c].cc"); Executor.mapVirtualFile("a.cc", "void x() {}"); Executor.mapVirtualFile("b.cc", "void y() {}"); Executor.mapVirtualFile("c.cc", "void z() {}"); + Executor.mapVirtualFile("ignore.cc", "void d() {}"); auto Err = Executor.execute(std::unique_ptr( new ReportResultActionFactory(Executor.getExecutionContext(; ASSERT_TRUE(!Err); EXPECT_THAT( Executor.getToolResults()->AllKVResults(), ::testing::UnorderedElementsAre(Named("x"), Named("y"), Named("z"))); + Filter.setValue(".*"); // reset to default value. } TEST(AllTUsToolTest, ManyFiles) { Index: include/clang/Tooling/AllTUsExecution.h === --- include/clang/Tooling/AllTUsExecution.h +++ include/clang/Tooling/AllTUsExecution.h @@ -72,6 +72,8 @@ unsigned ThreadCount; }; +ext
r346131 - [Tooling] Add "-filter" option to AllTUsExecution
Author: hokein Date: Mon Nov 5 05:42:05 2018 New Revision: 346131 URL: http://llvm.org/viewvc/llvm-project?rev=346131&view=rev Log: [Tooling] Add "-filter" option to AllTUsExecution Summary: We can run the tools on a subset files of compilation database. Reviewers: ioeric Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D54092 Modified: cfe/trunk/include/clang/Tooling/AllTUsExecution.h cfe/trunk/lib/Tooling/AllTUsExecution.cpp cfe/trunk/unittests/Tooling/ExecutionTest.cpp Modified: cfe/trunk/include/clang/Tooling/AllTUsExecution.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/AllTUsExecution.h?rev=346131&r1=346130&r2=346131&view=diff == --- cfe/trunk/include/clang/Tooling/AllTUsExecution.h (original) +++ cfe/trunk/include/clang/Tooling/AllTUsExecution.h Mon Nov 5 05:42:05 2018 @@ -72,6 +72,8 @@ private: unsigned ThreadCount; }; +extern llvm::cl::opt Filter; + } // end namespace tooling } // end namespace clang Modified: cfe/trunk/lib/Tooling/AllTUsExecution.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/AllTUsExecution.cpp?rev=346131&r1=346130&r2=346131&view=diff == --- cfe/trunk/lib/Tooling/AllTUsExecution.cpp (original) +++ cfe/trunk/lib/Tooling/AllTUsExecution.cpp Mon Nov 5 05:42:05 2018 @@ -53,6 +53,12 @@ private: } // namespace +llvm::cl::opt +Filter("filter", + llvm::cl::desc("Only process files that match this filter. " + "This flag only applies to all-TUs."), + llvm::cl::init(".*")); + AllTUsToolExecutor::AllTUsToolExecutor( const CompilationDatabase &Compilations, unsigned ThreadCount, std::shared_ptr PCHContainerOps) @@ -110,7 +116,10 @@ llvm::Error AllTUsToolExecutor::execute( llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } +llvm::Regex RegexFilter(Filter); for (std::string File : Files) { + if (!RegexFilter.match(File)) +continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + @@ -147,7 +156,8 @@ llvm::Error AllTUsToolExecutor::execute( static llvm::cl::opt ExecutorConcurrency( "execute-concurrency", llvm::cl::desc("The number of threads used to process all files in " - "parallel. Set to 0 for hardware concurrency."), + "parallel. Set to 0 for hardware concurrency. " + "This flag only applies to all-TUs."), llvm::cl::init(0)); class AllTUsToolExecutorPlugin : public ToolExecutorPlugin { Modified: cfe/trunk/unittests/Tooling/ExecutionTest.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/ExecutionTest.cpp?rev=346131&r1=346130&r2=346131&view=diff == --- cfe/trunk/unittests/Tooling/ExecutionTest.cpp (original) +++ cfe/trunk/unittests/Tooling/ExecutionTest.cpp Mon Nov 5 05:42:05 2018 @@ -248,12 +248,14 @@ private: MATCHER_P(Named, Name, "") { return arg.first == Name; } TEST(AllTUsToolTest, AFewFiles) { - FixedCompilationDatabaseWithFiles Compilations(".", {"a.cc", "b.cc", "c.cc"}, - std::vector()); + FixedCompilationDatabaseWithFiles Compilations( + ".", {"a.cc", "b.cc", "c.cc", "ignore.cc"}, std::vector()); AllTUsToolExecutor Executor(Compilations, /*ThreadCount=*/0); + Filter.setValue("[a-c].cc"); Executor.mapVirtualFile("a.cc", "void x() {}"); Executor.mapVirtualFile("b.cc", "void y() {}"); Executor.mapVirtualFile("c.cc", "void z() {}"); + Executor.mapVirtualFile("ignore.cc", "void d() {}"); auto Err = Executor.execute(std::unique_ptr( new ReportResultActionFactory(Executor.getExecutionContext(; @@ -261,6 +263,7 @@ TEST(AllTUsToolTest, AFewFiles) { EXPECT_THAT( Executor.getToolResults()->AllKVResults(), ::testing::UnorderedElementsAre(Named("x"), Named("y"), Named("z"))); + Filter.setValue(".*"); // reset to default value. } TEST(AllTUsToolTest, ManyFiles) { ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D54077: [clangd] Implemented DraftFileSystem
On Mon, Nov 5, 2018, 13:58 Ilya Biryukov via Phabricator < revi...@reviews.llvm.org wrote: > ilya-biryukov added a comment. > > > There's the usual concern that the current behavior is reasonable and > people like it. > > I think it would be reasonable to say that a large portion of C++ users > are used to the behavior that this patch introduces. > Specifically, all IDEs (Clion, Eclipse, Visual Studio for certain, not > sure about XCode) will see the contents of the headers that the user typed > and not the other way around. > It does not cause any inconsistencies there, the IDE will save all your > open files when you hit "build". That also means reading files from the > filesystem gives non-useful errors in that case, because the build will > actually see the files from the editors. > I agree, which is why I described both behaviors as reasonable :-) I don't this this address either of my concerns, though. > > Repository: > rCTE Clang Tools Extra > > https://reviews.llvm.org/D54077 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64
krytarowski planned changes to this revision. krytarowski added a comment. OK. Repository: rL LLVM https://reviews.llvm.org/D34018 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.
Szelethus added a comment. I read through your patch multiple times, have read your mail on cfe-dev, and I still couldn't find eve minor nits, well done! However, both in the mail, and in this discussion, there are a lot of invaluable information about the inner workings of the analyzer, that I fear will be lost once this revision is closed, even though I don't think the main logic will change in the near future. It would be neat to document it for the next generation somewhere. Another thing, since we're directly testing the functionality of `SymbolReaper`, maybe it'd be worth to also include unit tests. I'll probably revisit this revision as I read through your followup patches, but I can't seem to find errors in the main logic straight away. GG! Repository: rC Clang https://reviews.llvm.org/D18860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54104: [Tooling] Correct the total number of files being processed when `filter` is provided.
hokein created this revision. hokein added a reviewer: ioeric. Repository: rC Clang https://reviews.llvm.org/D54104 Files: lib/Tooling/AllTUsExecution.cpp Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -96,7 +96,13 @@ llvm::errs() << Msg.str() << "\n"; }; - auto Files = Compilations.getAllFiles(); + std::vector Files; + llvm::Regex RegexFilter(Filter); + for (const auto& File : Compilations.getAllFiles()) { +if (!RegexFilter.match(File)) + continue; +Files.push_back(File); + } // Add a counter to track the progress. const std::string TotalNumStr = std::to_string(Files.size()); unsigned Counter = 0; @@ -118,8 +124,6 @@ } llvm::Regex RegexFilter(Filter); for (std::string File : Files) { - if (!RegexFilter.match(File)) -continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -96,7 +96,13 @@ llvm::errs() << Msg.str() << "\n"; }; - auto Files = Compilations.getAllFiles(); + std::vector Files; + llvm::Regex RegexFilter(Filter); + for (const auto& File : Compilations.getAllFiles()) { +if (!RegexFilter.match(File)) + continue; +Files.push_back(File); + } // Add a counter to track the progress. const std::string TotalNumStr = std::to_string(Files.size()); unsigned Counter = 0; @@ -118,8 +124,6 @@ } llvm::Regex RegexFilter(Filter); for (std::string File : Files) { - if (!RegexFilter.match(File)) -continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54104: [Tooling] Correct the total number of files being processed when `filter` is provided.
ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: lib/Tooling/AllTUsExecution.cpp:103 +if (!RegexFilter.match(File)) + continue; +Files.push_back(File); nit: `if (match) then push_back` would be a line shorter. Comment at: lib/Tooling/AllTUsExecution.cpp:125 } llvm::Regex RegexFilter(Filter); for (std::string File : Files) { This is unused now Repository: rC Clang https://reviews.llvm.org/D54104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54077: [clangd] Implemented DraftFileSystem
LutsenkoDanil planned changes to this revision. LutsenkoDanil added a comment. @ilya-biryukov, For example, VSCode saves all changed files by default when you press Ctrl+Shift+B (if build task configured for project). @klimek If behavior will be configurable, is it ok for you? @sammccall Current behavior may confuse new users, since, other IDEs mostly (all?) shows diagnostics for edited files instead of saved one. And it's unexpected that headers have 2 states - visible and saved (which cannot be viewed in IDE at all). Looks like performance will be same like usage of 'auto save after delay' feature in editor, if we make debounce delay configurable, what do you think? I suggest inroduce following changes to the patch: [ ] Changes proposed by @ilya-biryukov [ ] Configurable behavior [ ] Debounce timeout option Comment at: clangd/FS.h:82 + +if (auto D = DS.getDraft(Path.str())) { + return std::unique_ptr( ilya-biryukov wrote: > This assumes the `Path` is absolute and `vfs::FileSystem` can be called with > non-absolute paths too. > One way to make it work with relative paths is to create an > `InMemoryFileSystem` with the headers (it handles the absolute paths > conversions) and create an `OverlayFileSystem` on top of it and the > `RealFileSystem`. > > This would also make more complicated things work, e.g. getting the same > files when traversing parent directories, etc. > > Could we try this approach? WDYT? > Sounds good. Thank you for review! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54077: [clangd] Implemented DraftFileSystem
klimek added a comment. In https://reviews.llvm.org/D54077#1287282, @LutsenkoDanil wrote: > @klimek If behavior will be configurable, is it ok for you? I have the same concerns as Sam for making this an option. > @sammccall Current behavior may confuse new users, since, other IDEs mostly > (all?) shows diagnostics for edited files instead of saved one. And it's > unexpected that headers have 2 states - visible and saved (which cannot be > viewed in IDE at all). Looks like performance will be same like usage of > 'auto save after delay' feature in editor, if we make debounce delay > configurable, what do you think? don't most IDEs show whether a file is saved or just modified? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54104: [Tooling] Correct the total number of files being processed when `filter` is provided.
hokein updated this revision to Diff 172585. hokein marked 2 inline comments as done. hokein added a comment. Address comments. Repository: rC Clang https://reviews.llvm.org/D54104 Files: lib/Tooling/AllTUsExecution.cpp Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -96,7 +96,12 @@ llvm::errs() << Msg.str() << "\n"; }; - auto Files = Compilations.getAllFiles(); + std::vector Files; + llvm::Regex RegexFilter(Filter); + for (const auto& File : Compilations.getAllFiles()) { +if (RegexFilter.match(File)) + Files.push_back(File); + } // Add a counter to track the progress. const std::string TotalNumStr = std::to_string(Files.size()); unsigned Counter = 0; @@ -116,10 +121,7 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } -llvm::Regex RegexFilter(Filter); for (std::string File : Files) { - if (!RegexFilter.match(File)) -continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + Index: lib/Tooling/AllTUsExecution.cpp === --- lib/Tooling/AllTUsExecution.cpp +++ lib/Tooling/AllTUsExecution.cpp @@ -96,7 +96,12 @@ llvm::errs() << Msg.str() << "\n"; }; - auto Files = Compilations.getAllFiles(); + std::vector Files; + llvm::Regex RegexFilter(Filter); + for (const auto& File : Compilations.getAllFiles()) { +if (RegexFilter.match(File)) + Files.push_back(File); + } // Add a counter to track the progress. const std::string TotalNumStr = std::to_string(Files.size()); unsigned Counter = 0; @@ -116,10 +121,7 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } -llvm::Regex RegexFilter(Filter); for (std::string File : Files) { - if (!RegexFilter.match(File)) -continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346135 - [Tooling] Correct the total number of files being processed when `filter` is provided.
Author: hokein Date: Mon Nov 5 07:08:00 2018 New Revision: 346135 URL: http://llvm.org/viewvc/llvm-project?rev=346135&view=rev Log: [Tooling] Correct the total number of files being processed when `filter` is provided. Reviewers: ioeric Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D54104 Modified: cfe/trunk/lib/Tooling/AllTUsExecution.cpp Modified: cfe/trunk/lib/Tooling/AllTUsExecution.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/AllTUsExecution.cpp?rev=346135&r1=346134&r2=346135&view=diff == --- cfe/trunk/lib/Tooling/AllTUsExecution.cpp (original) +++ cfe/trunk/lib/Tooling/AllTUsExecution.cpp Mon Nov 5 07:08:00 2018 @@ -96,7 +96,12 @@ llvm::Error AllTUsToolExecutor::execute( llvm::errs() << Msg.str() << "\n"; }; - auto Files = Compilations.getAllFiles(); + std::vector Files; + llvm::Regex RegexFilter(Filter); + for (const auto& File : Compilations.getAllFiles()) { +if (RegexFilter.match(File)) + Files.push_back(File); + } // Add a counter to track the progress. const std::string TotalNumStr = std::to_string(Files.size()); unsigned Counter = 0; @@ -116,10 +121,7 @@ llvm::Error AllTUsToolExecutor::execute( llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } -llvm::Regex RegexFilter(Filter); for (std::string File : Files) { - if (!RegexFilter.match(File)) -continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54104: [Tooling] Correct the total number of files being processed when `filter` is provided.
This revision was automatically updated to reflect the committed changes. Closed by commit rL346135: [Tooling] Correct the total number of files being processed when `filter` is… (authored by hokein, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D54104 Files: cfe/trunk/lib/Tooling/AllTUsExecution.cpp Index: cfe/trunk/lib/Tooling/AllTUsExecution.cpp === --- cfe/trunk/lib/Tooling/AllTUsExecution.cpp +++ cfe/trunk/lib/Tooling/AllTUsExecution.cpp @@ -96,7 +96,12 @@ llvm::errs() << Msg.str() << "\n"; }; - auto Files = Compilations.getAllFiles(); + std::vector Files; + llvm::Regex RegexFilter(Filter); + for (const auto& File : Compilations.getAllFiles()) { +if (RegexFilter.match(File)) + Files.push_back(File); + } // Add a counter to track the progress. const std::string TotalNumStr = std::to_string(Files.size()); unsigned Counter = 0; @@ -116,10 +121,7 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } -llvm::Regex RegexFilter(Filter); for (std::string File : Files) { - if (!RegexFilter.match(File)) -continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + Index: cfe/trunk/lib/Tooling/AllTUsExecution.cpp === --- cfe/trunk/lib/Tooling/AllTUsExecution.cpp +++ cfe/trunk/lib/Tooling/AllTUsExecution.cpp @@ -96,7 +96,12 @@ llvm::errs() << Msg.str() << "\n"; }; - auto Files = Compilations.getAllFiles(); + std::vector Files; + llvm::Regex RegexFilter(Filter); + for (const auto& File : Compilations.getAllFiles()) { +if (RegexFilter.match(File)) + Files.push_back(File); + } // Add a counter to track the progress. const std::string TotalNumStr = std::to_string(Files.size()); unsigned Counter = 0; @@ -116,10 +121,7 @@ llvm::errs() << "Error while getting current working directory: " << EC.message() << "\n"; } -llvm::Regex RegexFilter(Filter); for (std::string File : Files) { - if (!RegexFilter.match(File)) -continue; Pool.async( [&](std::string Path) { Log("[" + std::to_string(Count()) + "/" + TotalNumStr + ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346124 - [mips][msa] Fix broken test
Author: abeserminji Date: Mon Nov 5 02:22:51 2018 New Revision: 346124 URL: http://llvm.org/viewvc/llvm-project?rev=346124&view=rev Log: [mips][msa] Fix broken test Test builtins-mips-msa-error.c wasn't reporting errors. This patch fixes the test, so further test cases can be added. Differential Revision: https://reviews.llvm.org/D53984 Modified: cfe/trunk/test/CodeGen/builtins-mips-msa-error.c Modified: cfe/trunk/test/CodeGen/builtins-mips-msa-error.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtins-mips-msa-error.c?rev=346124&r1=346123&r2=346124&view=diff == --- cfe/trunk/test/CodeGen/builtins-mips-msa-error.c (original) +++ cfe/trunk/test/CodeGen/builtins-mips-msa-error.c Mon Nov 5 02:22:51 2018 @@ -1,18 +1,22 @@ // REQUIRES: mips-registered-target -// RUN: not %clang_cc1 -triple mips-unknown-linux-gnu -fsyntax-only %s \ +// RUN: %clang_cc1 -triple mips-unknown-linux-gnu -fsyntax-only %s \ // RUN:-target-feature +msa -target-feature +fp64 \ -// RUN:-mfloat-abi hard -o - 2>&1 | FileCheck %s +// RUN:-verify -mfloat-abi hard -o - 2>&1 #include void test(void) { v16i8 v16i8_a = (v16i8) {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; + v16i8 v16i8_b = (v16i8) {16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}; v16i8 v16i8_r; v8i16 v8i16_a = (v8i16) {0, 1, 2, 3, 4, 5, 6, 7}; + v8i16 v8i16_b = (v8i16) {8, 9, 10, 11, 12, 13, 14, 15}; v8i16 v8i16_r; v4i32 v4i32_a = (v4i32) {0, 1, 2, 3}; + v4i32 v4i32_b = (v4i32) {4, 5, 6, 7}; v4i32 v4i32_r; v2i64 v2i64_a = (v2i64) {0, 1}; + v2i64 v2i64_b = (v2i64) {3, 4}; v2i64 v2i64_r; v16u8 v16u8_a = (v16u8) {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; @@ -24,388 +28,385 @@ void test(void) { v2u64 v2u64_a = (v2u64) {0, 1}; v2u64 v2u64_r; - int int_r; long long ll_r; - v16u8_r = __msa_addvi_b(v16u8_a, 32); // expected-error {{argument should be a value from 0 to 31}} - v8u16_r = __msa_addvi_h(v8u16_a, 32); // expected-error {{argument should be a value from 0 to 31}} - v4u32_r = __msa_addvi_w(v4u32_a, 32); // expected-error {{argument should be a value from 0 to 31}} - v2u64_r = __msa_addvi_d(v2u64_a, 32); // expected-error {{argument should be a value from 0 to 31}} - - v16i8_r = __msa_andi_b(v16i8_a, 256); // CHECK: warning: argument should be a value from 0 to 255}} - v8i16_r = __msa_andi_b(v8i16_a, 256); // CHECK: warning: argument should be a value from 0 to 255}} - v4i32_r = __msa_andi_b(v4i32_a, 256); // CHECK: warning: argument should be a value from 0 to 255}} - v2i64_r = __msa_andi_b(v2i64_a, 256); // CHECK: warning: argument should be a value from 0 to 255}} - - v16i8_r = __msa_bclri_b(v16i8_a, 8); // expected-error {{argument should be a value from 0 to 7}} - v8i16_r = __msa_bclri_h(v8i16_a, 16); // expected-error {{argument should be a value from 0 to 15}} - v4i32_r = __msa_bclri_w(v4i32_a, 33); // expected-error {{argument should be a value from 0 to 31}} - v2i64_r = __msa_bclri_d(v2i64_a, 64); // expected-error {{argument should be a value from 0 to 63}} - - v16i8_r = __msa_binsli_b(v16i8_r, v16i8_a, 8); // expected-error {{argument should be a value from 0 to 7}} - v8i16_r = __msa_binsli_h(v8i16_r, v8i16_a, 16);// expected-error {{argument should be a value from 0 to 15}} - v4i32_r = __msa_binsli_w(v4i32_r, v4i32_a, 32);// expected-error {{argument should be a value from 0 to 31}} - v2i64_r = __msa_binsli_d(v2i64_r, v2i64_a, 64);// expected-error {{argument should be a value from 0 to 63}} - - v16i8_r = __msa_binsri_b(v16i8_r, v16i8_a, 8); // expected-error {{argument should be a value from 0 to 7}} - v8i16_r = __msa_binsri_h(v8i16_r, v8i16_a, 16);// expected-error {{argument should be a value from 0 to 15}} - v4i32_r = __msa_binsri_w(v4i32_r, v4i32_a, 32);// expected-error {{argument should be a value from 0 to 31}} - v2i64_r = __msa_binsri_d(v2i64_r, v2i64_a, 64);// expected-error {{argument should be a value from 0 to 63}} - - v16i8_r = __msa_bmnzi_b(v16i8_r, v16i8_a, 256);// expected-error {{argument should be a value from 0 to 255}} - - v16i8_r = __msa_bmzi_b(v16i8_r, v16i8_a, 256); // expected-error {{argument should be a value from 0 to 255}} - - v16i8_r = __msa_bnegi_b(v16i8_a, 8); // expected-error {{argument should be a value from 0 to 7}} - v8i16_r = __msa_bnegi_h(v8i16_a, 16); // expected-error {{argument should be a value from 0 to 15}} - v4i32_r = __msa_bnegi_w(v4i32_a, 32); // expected-error {{argument should be a value from 0 to 31}} - v2i64_r = __msa_bnegi_d(v2i64_a, 64); // expected-error {{argument should be a
[PATCH] D54077: [clangd] Implemented DraftFileSystem
simark added a comment. In https://reviews.llvm.org/D54077#1287153, @klimek wrote: > I'm in yet another camp: I carefully save when I have something that is > correct enough syntax, so I only want errors from with changes from the exact > file I'm editing and the rest of the files in saved state. That sounds like the current behavior, doesn't it? Personally, I would like the behavior proposed by this patch. I think it perfectly in line with the idea of showing the same diagnostics as the compiler would produce. It's just that it's what the compiler would produce if compiling the files as seen in the editor. Also, in the LSP, after a `didOpen` and until the corresponding `didClose`, the source of truth for that file is supposed to be what is in the editor buffer. So I think it would make sense that if other files include the edited file, they should see the content from the editor. Of course, that is dependent of having an efficient cancelling mechanism. Even with some debouncing, an update to a header file can trigger the re-processing of many TUs, so if I do another edit to the same header shortly after, all the queued jobs from the first edit should be dropped. But I think we already have that, don't we? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53982: Output "rule" information in SARIF
aaron.ballman added inline comments. Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242 +#define GET_CHECKERS +#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ + .Case(FULLNAME, HELPTEXT) +#include "clang/StaticAnalyzer/Checkers/Checkers.inc" +#undef CHECKER +#undef GET_CHECKERS Szelethus wrote: > Szelethus wrote: > > Hmmm, this won't handle checkers loaded from plugins. > I don't immediately know the solution is to this, but when you invoke clang > with `-cc1 -analyzer-checker-help`, plugin checkers are also displayed, and > [[ > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp#L150 > | this is line that magically does it ]]. > Maybe store the plugins in `AnalyzerOptions`, and move `ClangCheckerRegistry` > to `include/clang/StaticAnalyzer/Frontend`? Oof, this is a good point. Thank you for bringing it up! I'll look into this as soon as I have the chance. Do we have any tests that use plugins (preferably on Windows) so that I have a way to test this functionality out? https://reviews.llvm.org/D53982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
aaron.ballman added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); NoQ wrote: > aaron.ballman wrote: > > NoQ wrote: > > > aaron.ballman wrote: > > > > Szelethus wrote: > > > > > Szelethus wrote: > > > > > > ZaMaZaN4iK wrote: > > > > > > > lebedev.ri wrote: > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > lebedev.ri wrote: > > > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > > > lebedev.ri wrote: > > > > > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > > > > > aaron.ballman wrote: > > > > > > > > > > > > > > `const auto *` > > > > > > > > > > > > > Why do we need this change here? If I understand > > > > > > > > > > > > > correctly, with `const auto*` we also need change > > > > > > > > > > > > > initializer to > > > > > > > > > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > > > > > > > > > But I don't understand why we need this. > > > > > > > > > > > > Is `ValueToCastOptional` a pointer, a reference, or > > > > > > > > > > > > just an actual `DefinedOrUnknownSVal`? I can't tell. > > > > > > > > > > > > (sidenote: would be great to have a clang-tidy check > > > > > > > > > > > > for this.) > > > > > > > > > > > `ValueToCastOptional` is > > > > > > > > > > > `llvm::Optional` > > > > > > > > > > See, all my guesses were wrong. That is why it should not > > > > > > > > > > be `auto` at all. > > > > > > > > > I don't agree with you for this case. Honestly it's like a > > > > > > > > > yet another holywar question. If we are talking only about > > > > > > > > > this case - here you can see `getAs` > > > > > > > > > part of the expression. this means clearly (at least for me) > > > > > > > > > that we get something like `DefinedOrUnknownSVal`. What we > > > > > > > > > get? I just press hotkey in my favourite IDE/text editor and > > > > > > > > > see that `getAs` returns > > > > > > > > > `llvm::Optional`. From my point of view > > > > > > > > > it's clear enough here. > > > > > > > > > > > > > > > > > > If we are talking more generally about question "When should > > > > > > > > > we use `auto` at all? " - we can talk, but not here, I think > > > > > > > > > :) > > > > > > > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > > > > > > > > comes to mind. > > > > > > > > > What we get? I just press hotkey in my favourite IDE/text > > > > > > > > > editor and see that getAs returns > > > > > > > > > llvm::Optional > > > > > > > > Which hotkey do i need to press to see this here, in the > > > > > > > > phabricator? > > > > > > > > > > > > > > > > This really shouldn't be `auto`, if you have to explain that in > > > > > > > > the variable's name, justify it in review comments. > > > > > > > Ok, didn't know about such LLVM coding standard. Of course, with > > > > > > > this information I will fix using `auto` here. Thank you. > > > > > > Actually, I disagree. In the Static Analyzer we use `auto` if the > > > > > > return type is in the name of the expression, and `getAs` may fail, > > > > > > so it returns an `Optional`. In the case where a nullptr may be > > > > > > returned to signal failure, `auto *` is used, so I believe that > > > > > > `auto` is appropriate here. > > > > > But don't change it back now, it doesn't matter a whole lot :D > > > > > Actually, I disagree. In the Static Analyzer we use auto if the > > > > > return type is in the name of the expression, and getAs may fail, so > > > > > it returns an Optional. > > > > > > > > The static analyzer should be following the same coding standard as the > > > > rest of the project. This is new code, so it's not matching > > > > inappropriate styles from older code, so there's really no reason to > > > > not match the coding standard. > > > > > > > > > In the case where a nullptr may be returned to signal failure, auto * > > > > > is used, so I believe that auto is appropriate here. > > > > > > > > I disagree that `auto` is appropriate here. The amount of confusion > > > > about the type demonstrates why. > > > I confirm the strong local tradition of preferring `auto` for optional > > > `SVal`s in new code, and i believe it's well-justified from the overall > > > coding guideline's point of view. Even if you guys didn't get it right > > > from the start, the amount of Analyzer-specific experience required to > > > understand that it's an optional is very minimal and people get used to > > > it very quickly when they start actively working on the codebase. > > > > > > On one hand, being a class that combines LLVM custom RTTI with > > > pass-by-value semantics (i.e., it's like `QualType` when it comes to > > > passing it around and like `Type` when it comes to casting), optionals > > > are inevitable for representing `SVal` dynamic ca
[PATCH] D54105: [clangd] Deduplicate query scopes.
ioeric created this revision. ioeric added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay. For example, when anonymous namespace is present, duplicated namespaces might be generated for the enclosing namespace. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54105 Files: clangd/CodeComplete.cpp unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.cpp === --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -1142,6 +1142,23 @@ UnorderedElementsAre(""; } +TEST(CompletionTest, NoDuplicatedQueryScopes) { + auto Requests = captureIndexRequests(R"cpp( + namespace {} + + namespace na { + namespace {} + namespace nb { + ^ + } // namespace nb + } // namespace na + )cpp"); + + EXPECT_THAT(Requests, + ElementsAre(Field(&FuzzyFindRequest::Scopes, +UnorderedElementsAre("na::", "na::nb::", ""; +} + TEST(CompletionTest, NoIndexCompletionsInsideClasses) { auto Completions = completions( R"cpp( Index: clangd/CodeComplete.cpp === --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -538,16 +538,14 @@ // Set if the qualifier is not fully resolved by Sema. Optional UnresolvedQualifier; - // Construct scopes being queried in indexes. + // Construct scopes being queried in indexes. The results are deduplicated. // This method format the scopes to match the index request representation. std::vector scopesForIndexQuery() { -std::vector Results; -for (StringRef AS : AccessibleScopes) { - Results.push_back(AS); - if (UnresolvedQualifier) -Results.back() += *UnresolvedQualifier; -} -return Results; +std::set Results; +for (StringRef AS : AccessibleScopes) + Results.insert( + ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str()); +return {Results.begin(), Results.end()}; } }; Index: unittests/clangd/CodeCompleteTests.cpp === --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -1142,6 +1142,23 @@ UnorderedElementsAre(""; } +TEST(CompletionTest, NoDuplicatedQueryScopes) { + auto Requests = captureIndexRequests(R"cpp( + namespace {} + + namespace na { + namespace {} + namespace nb { + ^ + } // namespace nb + } // namespace na + )cpp"); + + EXPECT_THAT(Requests, + ElementsAre(Field(&FuzzyFindRequest::Scopes, +UnorderedElementsAre("na::", "na::nb::", ""; +} + TEST(CompletionTest, NoIndexCompletionsInsideClasses) { auto Completions = completions( R"cpp( Index: clangd/CodeComplete.cpp === --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -538,16 +538,14 @@ // Set if the qualifier is not fully resolved by Sema. Optional UnresolvedQualifier; - // Construct scopes being queried in indexes. + // Construct scopes being queried in indexes. The results are deduplicated. // This method format the scopes to match the index request representation. std::vector scopesForIndexQuery() { -std::vector Results; -for (StringRef AS : AccessibleScopes) { - Results.push_back(AS); - if (UnresolvedQualifier) -Results.back() += *UnresolvedQualifier; -} -return Results; +std::set Results; +for (StringRef AS : AccessibleScopes) + Results.insert( + ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str()); +return {Results.begin(), Results.end()}; } }; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53926: [clangd] Only add global scope to completion query scopes for TU context.
ioeric abandoned this revision. ioeric marked an inline comment as done. ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:563 for (auto *Context : CCContext.getVisitedContexts()) { - if (isa(Context)) + if (isa(Context)) { Info.AccessibleScopes.push_back(""); // global namespace ilya-biryukov wrote: > ioeric wrote: > > ilya-biryukov wrote: > > > Anonymous namespace inside other namespaces will also produce duplicate > > > scopes. > > > Maybe simply remove the duplicates from the vector before we return it? > > `printNamespaceScope()` will return "" for all anonymous namespaces, which > > should be covered as well. > I don't think that's the case. > My understanding is that > `printNamespaceScope("foobar::")` will return > `"foo::bar::"`. Thanks for the catch! Switched to deduplicate scopes instead (D54105) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53982: Output "rule" information in SARIF
Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242 +#define GET_CHECKERS +#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ + .Case(FULLNAME, HELPTEXT) +#include "clang/StaticAnalyzer/Checkers/Checkers.inc" +#undef CHECKER +#undef GET_CHECKERS aaron.ballman wrote: > Szelethus wrote: > > Szelethus wrote: > > > Hmmm, this won't handle checkers loaded from plugins. > > I don't immediately know the solution is to this, but when you invoke clang > > with `-cc1 -analyzer-checker-help`, plugin checkers are also displayed, and > > [[ > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp#L150 > > | this is line that magically does it ]]. > > Maybe store the plugins in `AnalyzerOptions`, and move > > `ClangCheckerRegistry` to `include/clang/StaticAnalyzer/Frontend`? > Oof, this is a good point. Thank you for bringing it up! I'll look into this > as soon as I have the chance. > > Do we have any tests that use plugins (preferably on Windows) so that I have > a way to test this functionality out? I'm working on the exact same issue, which is why I spotted this! There are some in `unittests/`, but I plan on enhancing them //a lot// in the coming days. I'll make sure to add you as subscriber on related paches, until then, unless users of the Saris output are likely to use the StaticAnalyzer with plugins, I think feel free to wait on my progress. I expect to come out with a patch during the week. But you never know, I guess. https://github.com/llvm-mirror/clang/blob/master/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp https://reviews.llvm.org/D53982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53933: [clangd] Get rid of QueryScopes.empty() == AnyScope special case.
ioeric updated this revision to Diff 172591. ioeric marked an inline comment as done. ioeric added a comment. - Remove addressed FIXME. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53933 Files: clangd/FindSymbols.cpp clangd/index/Index.h clangd/index/MemIndex.cpp clangd/index/dex/Dex.cpp unittests/clangd/DexTests.cpp unittests/clangd/IndexTests.cpp unittests/clangd/SyncAPI.cpp Index: unittests/clangd/SyncAPI.cpp === --- unittests/clangd/SyncAPI.cpp +++ unittests/clangd/SyncAPI.cpp @@ -129,6 +129,7 @@ SymbolSlab runFuzzyFind(const SymbolIndex &Index, StringRef Query) { FuzzyFindRequest Req; Req.Query = Query; + Req.AnyScope = true; return runFuzzyFind(Index, Req); } Index: unittests/clangd/IndexTests.cpp === --- unittests/clangd/IndexTests.cpp +++ unittests/clangd/IndexTests.cpp @@ -90,14 +90,16 @@ symbol("2") /* duplicate */}; FuzzyFindRequest Req; Req.Query = "2"; + Req.AnyScope = true; MemIndex I(Symbols, RefSlab()); EXPECT_THAT(match(I, Req), ElementsAre("2")); } TEST(MemIndexTest, MemIndexLimitedNumMatches) { auto I = MemIndex::build(generateNumSymbols(0, 100), RefSlab()); FuzzyFindRequest Req; Req.Query = "5"; + Req.AnyScope = true; Req.Limit = 3; bool Incomplete; auto Matches = match(*I, Req, &Incomplete); @@ -112,6 +114,7 @@ RefSlab()); FuzzyFindRequest Req; Req.Query = "lol"; + Req.AnyScope = true; Req.Limit = 2; EXPECT_THAT(match(*I, Req), UnorderedElementsAre("LaughingOutLoud", "LittleOldLady")); @@ -122,6 +125,7 @@ MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab()); FuzzyFindRequest Req; Req.Query = "y"; + Req.AnyScope = true; EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3")); } Index: unittests/clangd/DexTests.cpp === --- unittests/clangd/DexTests.cpp +++ unittests/clangd/DexTests.cpp @@ -485,6 +485,7 @@ UnorderedElementsAre("other::A", "other::ABC")); Req.Query = ""; Req.Scopes = {}; + Req.AnyScope = true; EXPECT_THAT(match(*Index, Req), UnorderedElementsAre("ns::ABC", "ns::BCD", "::ABC", "ns::nested::ABC", "other::ABC", @@ -496,14 +497,16 @@ symbol("2") /* duplicate */}; FuzzyFindRequest Req; Req.Query = "2"; + Req.AnyScope = true; Dex I(Symbols, RefSlab(), URISchemes); EXPECT_THAT(match(I, Req), ElementsAre("2")); } TEST(DexTest, DexLimitedNumMatches) { auto I = Dex::build(generateNumSymbols(0, 100), RefSlab(), URISchemes); FuzzyFindRequest Req; Req.Query = "5"; + Req.AnyScope = true; Req.Limit = 3; bool Incomplete; auto Matches = match(*I, Req, &Incomplete); @@ -518,6 +521,7 @@ RefSlab(), URISchemes); FuzzyFindRequest Req; Req.Query = "lol"; + Req.AnyScope = true; Req.Limit = 2; EXPECT_THAT(match(*I, Req), UnorderedElementsAre("LaughingOutLoud", "LittleOldLady")); @@ -527,6 +531,7 @@ auto I = Dex::build(generateSymbols({"OneTwoThreeFour"}), RefSlab(), URISchemes); FuzzyFindRequest Req; + Req.AnyScope = true; bool Incomplete; EXPECT_THAT(match(*I, Req, &Incomplete), ElementsAre("OneTwoThreeFour")); @@ -549,6 +554,7 @@ auto I = Dex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(), URISchemes); FuzzyFindRequest Req; + Req.AnyScope = true; Req.Query = "y"; EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3")); } @@ -593,9 +599,9 @@ auto I = Dex::build(generateSymbols({"a::y1", "a::b::y2", "c::y3"}), RefSlab(), URISchemes); FuzzyFindRequest Req; + Req.AnyScope = true; Req.Query = "y"; Req.Scopes = {"a::"}; - Req.AnyScope = true; EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "a::b::y2", "c::y3")); } @@ -635,6 +641,7 @@ std::vector Symbols{CodeCompletionSymbol, NonCodeCompletionSymbol}; Dex I(Symbols, RefSlab(), URISchemes); FuzzyFindRequest Req; + Req.AnyScope = true; Req.RestrictForCodeCompletion = false; EXPECT_THAT(match(I, Req), ElementsAre("Completion", "NoCompletion")); Req.RestrictForCodeCompletion = true; @@ -651,6 +658,7 @@ Dex I(Symbols, RefSlab(), URISchemes); FuzzyFindRequest Req; + Req.AnyScope = true; Req.Query = "abc"; // The best candidate can change depending on the proximity paths. Req.Limit = 1; Index: clangd/index/dex/Dex.cpp === --- clangd/index/dex/Dex.cpp +++ clangd/index/dex/Dex.cpp @@ -178,7 +178,7 @@ std::vector> ScopeIterators; for (const auto &Scope : Req.Scopes) ScopeIterators.push_back(iterator(Token(Token::Kind::Scope, Scope))); - if
[PATCH] D53982: Output "rule" information in SARIF
aaron.ballman added inline comments. Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242 +#define GET_CHECKERS +#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ + .Case(FULLNAME, HELPTEXT) +#include "clang/StaticAnalyzer/Checkers/Checkers.inc" +#undef CHECKER +#undef GET_CHECKERS Szelethus wrote: > aaron.ballman wrote: > > Szelethus wrote: > > > Szelethus wrote: > > > > Hmmm, this won't handle checkers loaded from plugins. > > > I don't immediately know the solution is to this, but when you invoke > > > clang with `-cc1 -analyzer-checker-help`, plugin checkers are also > > > displayed, and [[ > > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp#L150 > > > | this is line that magically does it ]]. > > > Maybe store the plugins in `AnalyzerOptions`, and move > > > `ClangCheckerRegistry` to `include/clang/StaticAnalyzer/Frontend`? > > Oof, this is a good point. Thank you for bringing it up! I'll look into > > this as soon as I have the chance. > > > > Do we have any tests that use plugins (preferably on Windows) so that I > > have a way to test this functionality out? > I'm working on the exact same issue, which is why I spotted this! There are > some in `unittests/`, but I plan on enhancing them //a lot// in the coming > days. I'll make sure to add you as subscriber on related paches, until then, > unless users of the Saris output are likely to use the StaticAnalyzer with > plugins, I think feel free to wait on my progress. I expect to come out with > a patch during the week. But you never know, I guess. > > https://github.com/llvm-mirror/clang/blob/master/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp I'm fine with waiting. I don't expect to see heavy use of the Sarif exporter until the spec is finalized, as it's hard to write a Sarif viewer against a moving target. So we've got a few months to find these sort of things out. Thank you for CCing me on the related patches! https://reviews.llvm.org/D53982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54106: [clangd] Limit the index-returned results in dexp.
hokein created this revision. hokein added a reviewer: ioeric. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. When the limit of FuzzyFindRequest is not specified, MemIndex and DexIndex will set it MAX_UINT, which doesn't make sense in the active tool dexp (we might get lots results). This patch restricts it to a small number. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54106 Files: clangd/index/dex/dexp/Dexp.cpp Index: clangd/index/dex/dexp/Dexp.cpp === --- clangd/index/dex/dexp/Dexp.cpp +++ clangd/index/dex/dexp/Dexp.cpp @@ -52,6 +52,7 @@ std::vector getSymbolIDsFromIndex(StringRef QualifiedName, const SymbolIndex *Index) { FuzzyFindRequest Request; + Request.Limit = 10; // Remove leading "::" qualifier as FuzzyFind doesn't need leading "::" // qualifier for global scope. bool IsGlobalScope = QualifiedName.consume_front("::"); Index: clangd/index/dex/dexp/Dexp.cpp === --- clangd/index/dex/dexp/Dexp.cpp +++ clangd/index/dex/dexp/Dexp.cpp @@ -52,6 +52,7 @@ std::vector getSymbolIDsFromIndex(StringRef QualifiedName, const SymbolIndex *Index) { FuzzyFindRequest Request; + Request.Limit = 10; // Remove leading "::" qualifier as FuzzyFind doesn't need leading "::" // qualifier for global scope. bool IsGlobalScope = QualifiedName.consume_front("::"); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54106: [clangd] Limit the index-returned results in dexp.
ioeric added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:128 }; cl::opt Limit{ "limit", I think we should make it configurable like what we do here. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54106 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Still looking for feedback on the latest round of changes. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54109: [clang-query] continue even if files are skipped
Lekensteyn created this revision. Lekensteyn added reviewers: cfe-commits, pcc. Like clang-check, continue even if some source files were not found in the compilation database. This makes it possible to do search for matching sources and query files that exist in the compilation db: clang-query -c 'm callExpr(callee(functionDecl(hasName("function_name"' $(grep -rl ^function_name) Note: buildASTs calls ClangTool::run which returns 1 if any of the commands failed and 2 otherwise if any of the files were missing. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54109 Files: clang-query/tool/ClangQuery.cpp test/clang-query/Inputs/database_template.json test/clang-query/database-missing-entry.c Index: test/clang-query/database-missing-entry.c === --- /dev/null +++ test/clang-query/database-missing-entry.c @@ -0,0 +1,12 @@ +// RUN: rm -rf %t && mkdir -p %t/src %t/build +// RUN: sed 's|test_dir|%t|g' %S/Inputs/database_template.json > %t/build/compile_commands.json +// RUN: echo 'int A = AVAL;' > %t/src/a.c +// RUN: echo 'deliberate parsing error' > %t/src/b.c +// RUN: clang-query -p=%t/build -c "m integerLiteral()" %t/src/a.c %t/src/b.c %t/src/missing.c 2>&1 | FileCheck %s + +// Test that neither parse errors nor missing database entries prevent further processing. +// CHECK: deliberate parsing error +// CHECK: Skipping {{.*[/\\]}}missing.c. Compile command not found. +// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c. +// CHECK-NOT-EXIST: unable to handle compilation +// CHECK: a.c:1:9: note: "root" binds here Index: test/clang-query/Inputs/database_template.json === --- /dev/null +++ test/clang-query/Inputs/database_template.json @@ -0,0 +1,12 @@ +[ +{ + "directory": "test_dir/build", + "command": "clang -DAVAL=8 -o a.o test_dir/src/a.c", + "file": "test_dir/src/a.c" +}, +{ + "directory": "test_dir/build", + "command": "clang -o b.o test_dir/src/b.c", + "file": "test_dir/src/b.c" +} +] Index: clang-query/tool/ClangQuery.cpp === --- clang-query/tool/ClangQuery.cpp +++ clang-query/tool/ClangQuery.cpp @@ -100,8 +100,7 @@ ClangTool Tool(OptionsParser.getCompilations(), OptionsParser.getSourcePathList()); std::vector> ASTs; - if (Tool.buildASTs(ASTs) != 0) -return 1; + Tool.buildASTs(ASTs); QuerySession QS(ASTs); Index: test/clang-query/database-missing-entry.c === --- /dev/null +++ test/clang-query/database-missing-entry.c @@ -0,0 +1,12 @@ +// RUN: rm -rf %t && mkdir -p %t/src %t/build +// RUN: sed 's|test_dir|%t|g' %S/Inputs/database_template.json > %t/build/compile_commands.json +// RUN: echo 'int A = AVAL;' > %t/src/a.c +// RUN: echo 'deliberate parsing error' > %t/src/b.c +// RUN: clang-query -p=%t/build -c "m integerLiteral()" %t/src/a.c %t/src/b.c %t/src/missing.c 2>&1 | FileCheck %s + +// Test that neither parse errors nor missing database entries prevent further processing. +// CHECK: deliberate parsing error +// CHECK: Skipping {{.*[/\\]}}missing.c. Compile command not found. +// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c. +// CHECK-NOT-EXIST: unable to handle compilation +// CHECK: a.c:1:9: note: "root" binds here Index: test/clang-query/Inputs/database_template.json === --- /dev/null +++ test/clang-query/Inputs/database_template.json @@ -0,0 +1,12 @@ +[ +{ + "directory": "test_dir/build", + "command": "clang -DAVAL=8 -o a.o test_dir/src/a.c", + "file": "test_dir/src/a.c" +}, +{ + "directory": "test_dir/build", + "command": "clang -o b.o test_dir/src/b.c", + "file": "test_dir/src/b.c" +} +] Index: clang-query/tool/ClangQuery.cpp === --- clang-query/tool/ClangQuery.cpp +++ clang-query/tool/ClangQuery.cpp @@ -100,8 +100,7 @@ ClangTool Tool(OptionsParser.getCompilations(), OptionsParser.getSourcePathList()); std::vector> ASTs; - if (Tool.buildASTs(ASTs) != 0) -return 1; + Tool.buildASTs(ASTs); QuerySession QS(ASTs); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54109: [clang-query] continue even if files are skipped
Lekensteyn added inline comments. Comment at: test/clang-query/database-missing-entry.c:9 +// CHECK: deliberate parsing error +// CHECK: Skipping {{.*[/\\]}}missing.c. Compile command not found. +// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c. On trunk this fails tests because of https://reviews.llvm.org/D51729. Reverting that patch will properly ignore the missing file. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54109 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.
ioeric added inline comments. Comment at: clangd/index/Background.cpp:235 +IndexedFileDigests[Path] = FilesToUpdate.lookup(Path); +IndexedSymbols.update(Path, + make_unique(std::move(Syms).build()), kadircet wrote: > This call is already thread-safe, no need for it to be under lock. This is for making sure that the update of digest and index data is atomic. If FileSymbols update is not in the lock, we might get the wrong digest for the indexed symbols in corner cases (e.g. T1 updates digest -> T2 updates digest -> T2 update symbols -> T1 updates symbols ). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53433 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53223: AMDGPU: Add sram-ecc feature options
kzhuravl added a comment. ping https://reviews.llvm.org/D53223 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54106: [clangd] Limit the index-returned results in dexp.
hokein updated this revision to Diff 172598. hokein added a comment. Address review comment. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54106 Files: clangd/index/dex/dexp/Dexp.cpp Index: clangd/index/dex/dexp/Dexp.cpp === --- clangd/index/dex/dexp/Dexp.cpp +++ clangd/index/dex/dexp/Dexp.cpp @@ -50,8 +50,10 @@ } std::vector getSymbolIDsFromIndex(StringRef QualifiedName, -const SymbolIndex *Index) { +const SymbolIndex *Index, +unsigned Limit) { FuzzyFindRequest Request; + Request.Limit = Limit; // Remove leading "::" qualifier as FuzzyFind doesn't need leading "::" // qualifier for global scope. bool IsGlobalScope = QualifiedName.consume_front("::"); @@ -158,6 +160,11 @@ cl::opt Name{ "name", cl::desc("Qualified name to look up."), }; + cl::opt Limit{ + "limit", + cl::init(10), + cl::desc("Max results to display"), + }; void run() override { if (ID.getNumOccurrences() == 0 && Name.getNumOccurrences() == 0) { @@ -173,7 +180,7 @@ } IDs.push_back(*SID); } else { - IDs = getSymbolIDsFromIndex(Name, Index); + IDs = getSymbolIDsFromIndex(Name, Index, Limit); } LookupRequest Request; @@ -216,7 +223,7 @@ } IDs.push_back(*SID); } else { - IDs = getSymbolIDsFromIndex(Name, Index); + IDs = getSymbolIDsFromIndex(Name, Index, /*Limit=*/10); } RefsRequest RefRequest; RefRequest.IDs.insert(IDs.begin(), IDs.end()); Index: clangd/index/dex/dexp/Dexp.cpp === --- clangd/index/dex/dexp/Dexp.cpp +++ clangd/index/dex/dexp/Dexp.cpp @@ -50,8 +50,10 @@ } std::vector getSymbolIDsFromIndex(StringRef QualifiedName, -const SymbolIndex *Index) { +const SymbolIndex *Index, +unsigned Limit) { FuzzyFindRequest Request; + Request.Limit = Limit; // Remove leading "::" qualifier as FuzzyFind doesn't need leading "::" // qualifier for global scope. bool IsGlobalScope = QualifiedName.consume_front("::"); @@ -158,6 +160,11 @@ cl::opt Name{ "name", cl::desc("Qualified name to look up."), }; + cl::opt Limit{ + "limit", + cl::init(10), + cl::desc("Max results to display"), + }; void run() override { if (ID.getNumOccurrences() == 0 && Name.getNumOccurrences() == 0) { @@ -173,7 +180,7 @@ } IDs.push_back(*SID); } else { - IDs = getSymbolIDsFromIndex(Name, Index); + IDs = getSymbolIDsFromIndex(Name, Index, Limit); } LookupRequest Request; @@ -216,7 +223,7 @@ } IDs.push_back(*SID); } else { - IDs = getSymbolIDsFromIndex(Name, Index); + IDs = getSymbolIDsFromIndex(Name, Index, /*Limit=*/10); } RefsRequest RefRequest; RefRequest.IDs.insert(IDs.begin(), IDs.end()); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54106: [clangd] Limit the index-returned results in dexp.
hokein added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:128 }; cl::opt Limit{ "limit", ioeric wrote: > I think we should make it configurable like what we do here. Made it configurable for `Lookup`. `Limit` option is used to restrict the number for final results, it is reasonable for `FuzzyFind`, and `Lookup` But for `Refs`, it means the number of different symbols, not the number of final results, I still hard-code it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54106 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54110: [Format] Add debugging to ObjC language guesser
benhamilton created this revision. benhamilton added a reviewer: krasimir. Herald added a subscriber: cfe-commits. To handle diagnosing bugs where ObjCHeaderStyleGuesser guesses wrong, this diff adds a bit more debug logging to the Objective-C language guesser. Repository: rC Clang https://reviews.llvm.org/D54110 Files: lib/Format/Format.cpp Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -1504,16 +1504,19 @@ SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) override { assert(Style.Language == FormatStyle::LK_Cpp); -IsObjC = guessIsObjC(AnnotatedLines, Tokens.getKeywords()); +IsObjC = guessIsObjC(Env.getSourceManager(), AnnotatedLines, + Tokens.getKeywords()); tooling::Replacements Result; return {Result, 0}; } bool isObjC() { return IsObjC; } private: - static bool guessIsObjC(const SmallVectorImpl &AnnotatedLines, - const AdditionalKeywords &Keywords) { + static bool + guessIsObjC(const SourceManager &SourceManager, + const SmallVectorImpl &AnnotatedLines, + const AdditionalKeywords &Keywords) { // Keep this array sorted, since we are binary searching over it. static constexpr llvm::StringLiteral FoundationIdentifiers[] = { "CGFloat", @@ -1604,9 +1607,15 @@ TT_ObjCBlockLBrace, TT_ObjCBlockLParen, TT_ObjCDecl, TT_ObjCForIn, TT_ObjCMethodExpr, TT_ObjCMethodSpecifier, TT_ObjCProperty)) { + LLVM_DEBUG(llvm::dbgs() + << "Detected ObjC at location " + << FormatTok->Tok.getLocation().printToString( +SourceManager) + << " token: " << FormatTok->TokenText << " token type: " + << getTokenTypeName(FormatTok->Type) << "\n"); return true; } -if (guessIsObjC(Line->Children, Keywords)) +if (guessIsObjC(SourceManager, Line->Children, Keywords)) return true; } } Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -1504,16 +1504,19 @@ SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) override { assert(Style.Language == FormatStyle::LK_Cpp); -IsObjC = guessIsObjC(AnnotatedLines, Tokens.getKeywords()); +IsObjC = guessIsObjC(Env.getSourceManager(), AnnotatedLines, + Tokens.getKeywords()); tooling::Replacements Result; return {Result, 0}; } bool isObjC() { return IsObjC; } private: - static bool guessIsObjC(const SmallVectorImpl &AnnotatedLines, - const AdditionalKeywords &Keywords) { + static bool + guessIsObjC(const SourceManager &SourceManager, + const SmallVectorImpl &AnnotatedLines, + const AdditionalKeywords &Keywords) { // Keep this array sorted, since we are binary searching over it. static constexpr llvm::StringLiteral FoundationIdentifiers[] = { "CGFloat", @@ -1604,9 +1607,15 @@ TT_ObjCBlockLBrace, TT_ObjCBlockLParen, TT_ObjCDecl, TT_ObjCForIn, TT_ObjCMethodExpr, TT_ObjCMethodSpecifier, TT_ObjCProperty)) { + LLVM_DEBUG(llvm::dbgs() + << "Detected ObjC at location " + << FormatTok->Tok.getLocation().printToString( +SourceManager) + << " token: " << FormatTok->TokenText << " token type: " + << getTokenTypeName(FormatTok->Type) << "\n"); return true; } -if (guessIsObjC(Line->Children, Keywords)) +if (guessIsObjC(SourceManager, Line->Children, Keywords)) return true; } } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54110: [Format] Add debugging to ObjC language guesser
krasimir accepted this revision. krasimir added a comment. This revision is now accepted and ready to land. Helpful! Thanks! Repository: rC Clang https://reviews.llvm.org/D54110 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
Lekensteyn added a comment. Before this patch, missing compilation database entries resulted in "Skipping Compile command not found." which is assumed by the tests in this clang-query patch: https://reviews.llvm.org/D54109 After this patch, a command is inferred, but this is not always desired. Use case: find all function calls with a certain name using the compilation database produced by CMake. Example command: clang-query -p=/tmp/wsbuild -c 'set output print' -c 'm callExpr(callee(functionDecl(hasName("uat_new"' $(grep -rl uat_new epan/dissectors/) Expected result for some template files which do not exist in the compilation database: Skipping .../epan/dissectors/asn1/x509af/packet-x509af-template.c. Compile command not found. Actual result (clang-query tries to parse the contents which will fail since it is not an actual C source file): .../epan/dissectors/asn1/x509af/packet-x509af-template.c:18:10: fatal error: 'packet-ber.h' file not found I'm not entirely sure what to do here. The old behavior works great in cases where a complete database is available (produced by CMake). The new behavior might work better for clangd (?), but it breaks a use case (see above). Repository: rC Clang https://reviews.llvm.org/D51729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346144 - [Format] Add debugging to ObjC language guesser
Author: benhamilton Date: Mon Nov 5 08:59:33 2018 New Revision: 346144 URL: http://llvm.org/viewvc/llvm-project?rev=346144&view=rev Log: [Format] Add debugging to ObjC language guesser Summary: To handle diagnosing bugs where ObjCHeaderStyleGuesser guesses wrong, this diff adds a bit more debug logging to the Objective-C language guesser. Reviewers: krasimir Reviewed By: krasimir Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D54110 Modified: cfe/trunk/lib/Format/Format.cpp Modified: cfe/trunk/lib/Format/Format.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=346144&r1=346143&r2=346144&view=diff == --- cfe/trunk/lib/Format/Format.cpp (original) +++ cfe/trunk/lib/Format/Format.cpp Mon Nov 5 08:59:33 2018 @@ -1504,7 +1504,8 @@ public: SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) override { assert(Style.Language == FormatStyle::LK_Cpp); -IsObjC = guessIsObjC(AnnotatedLines, Tokens.getKeywords()); +IsObjC = guessIsObjC(Env.getSourceManager(), AnnotatedLines, + Tokens.getKeywords()); tooling::Replacements Result; return {Result, 0}; } @@ -1512,8 +1513,10 @@ public: bool isObjC() { return IsObjC; } private: - static bool guessIsObjC(const SmallVectorImpl &AnnotatedLines, - const AdditionalKeywords &Keywords) { + static bool + guessIsObjC(const SourceManager &SourceManager, + const SmallVectorImpl &AnnotatedLines, + const AdditionalKeywords &Keywords) { // Keep this array sorted, since we are binary searching over it. static constexpr llvm::StringLiteral FoundationIdentifiers[] = { "CGFloat", @@ -1604,9 +1607,15 @@ private: TT_ObjCBlockLBrace, TT_ObjCBlockLParen, TT_ObjCDecl, TT_ObjCForIn, TT_ObjCMethodExpr, TT_ObjCMethodSpecifier, TT_ObjCProperty)) { + LLVM_DEBUG(llvm::dbgs() + << "Detected ObjC at location " + << FormatTok->Tok.getLocation().printToString( +SourceManager) + << " token: " << FormatTok->TokenText << " token type: " + << getTokenTypeName(FormatTok->Type) << "\n"); return true; } -if (guessIsObjC(Line->Children, Keywords)) +if (guessIsObjC(SourceManager, Line->Children, Keywords)) return true; } } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54110: [Format] Add debugging to ObjC language guesser
This revision was automatically updated to reflect the committed changes. Closed by commit rC346144: [Format] Add debugging to ObjC language guesser (authored by benhamilton, committed by ). Changed prior to commit: https://reviews.llvm.org/D54110?vs=172599&id=172600#toc Repository: rC Clang https://reviews.llvm.org/D54110 Files: lib/Format/Format.cpp Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -1504,16 +1504,19 @@ SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) override { assert(Style.Language == FormatStyle::LK_Cpp); -IsObjC = guessIsObjC(AnnotatedLines, Tokens.getKeywords()); +IsObjC = guessIsObjC(Env.getSourceManager(), AnnotatedLines, + Tokens.getKeywords()); tooling::Replacements Result; return {Result, 0}; } bool isObjC() { return IsObjC; } private: - static bool guessIsObjC(const SmallVectorImpl &AnnotatedLines, - const AdditionalKeywords &Keywords) { + static bool + guessIsObjC(const SourceManager &SourceManager, + const SmallVectorImpl &AnnotatedLines, + const AdditionalKeywords &Keywords) { // Keep this array sorted, since we are binary searching over it. static constexpr llvm::StringLiteral FoundationIdentifiers[] = { "CGFloat", @@ -1604,9 +1607,15 @@ TT_ObjCBlockLBrace, TT_ObjCBlockLParen, TT_ObjCDecl, TT_ObjCForIn, TT_ObjCMethodExpr, TT_ObjCMethodSpecifier, TT_ObjCProperty)) { + LLVM_DEBUG(llvm::dbgs() + << "Detected ObjC at location " + << FormatTok->Tok.getLocation().printToString( +SourceManager) + << " token: " << FormatTok->TokenText << " token type: " + << getTokenTypeName(FormatTok->Type) << "\n"); return true; } -if (guessIsObjC(Line->Children, Keywords)) +if (guessIsObjC(SourceManager, Line->Children, Keywords)) return true; } } Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -1504,16 +1504,19 @@ SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) override { assert(Style.Language == FormatStyle::LK_Cpp); -IsObjC = guessIsObjC(AnnotatedLines, Tokens.getKeywords()); +IsObjC = guessIsObjC(Env.getSourceManager(), AnnotatedLines, + Tokens.getKeywords()); tooling::Replacements Result; return {Result, 0}; } bool isObjC() { return IsObjC; } private: - static bool guessIsObjC(const SmallVectorImpl &AnnotatedLines, - const AdditionalKeywords &Keywords) { + static bool + guessIsObjC(const SourceManager &SourceManager, + const SmallVectorImpl &AnnotatedLines, + const AdditionalKeywords &Keywords) { // Keep this array sorted, since we are binary searching over it. static constexpr llvm::StringLiteral FoundationIdentifiers[] = { "CGFloat", @@ -1604,9 +1607,15 @@ TT_ObjCBlockLBrace, TT_ObjCBlockLParen, TT_ObjCDecl, TT_ObjCForIn, TT_ObjCMethodExpr, TT_ObjCMethodSpecifier, TT_ObjCProperty)) { + LLVM_DEBUG(llvm::dbgs() + << "Detected ObjC at location " + << FormatTok->Tok.getLocation().printToString( +SourceManager) + << " token: " << FormatTok->TokenText << " token type: " + << getTokenTypeName(FormatTok->Type) << "\n"); return true; } -if (guessIsObjC(Line->Children, Keywords)) +if (guessIsObjC(SourceManager, Line->Children, Keywords)) return true; } } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.
Szelethus added a comment. Thanks for summarizing the problem so well in the summary! I should start doing that too. Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:343-346 + // There should have originally been a symbol. If it was a concrete value + // to begin with, the violation should have been handled immediately + // rather than delayed until constraints are updated. Why "symbol" rather than + // "region"? Because only symbolic regions can be null. To me, "should've been handled" sounds like that could be translated to an assert. Also, this doc seems confusing to me, but I might be wrong. You referenced the term "symbol" a couple types, but I don't see `SymbolVal` anywhere. Could you reword it a bit? Comment at: test/Analysis/nullability-arc.mm:1-2 +// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability -analyzer-output=text -verify %s +// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability -analyzer-output=text -verify %s -fobjc-arc + Let's organize this into multiple lines! ``` // RUN: %clang_analyze_cc1 -w -verify %s -fobjc-arc \ // RUN: -analyzer-checker=core,nullability -analyzer-output=text ``` Comment at: test/Analysis/nullability.mm:3-4 // RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=true -DNOSYSTEMHEADERS=1 -verify %s +// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -DNOSYSTEMHEADERS=0 -verify %s -fobjc-arc +// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=true -DNOSYSTEMHEADERS=1 -verify %s -fobjc-arc These too. Especially these :D Repository: rC Clang https://reviews.llvm.org/D54017 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54111: [clang-format] Do not threat the asm clobber [ as ObjCExpr
krasimir created this revision. Herald added a subscriber: cfe-commits. The opening square of an inline asm clobber was being annotated as an ObjCExpr. This caused, amongst other things, the ObjCGuesser to guess header files containing that pattern as ObjC files. Repository: rC Clang https://reviews.llvm.org/D54111 Files: lib/Format/TokenAnnotator.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -12753,6 +12753,15 @@ guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);")); } +TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) { + EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", + "void f() {\n" + " asm (\"mov %[e], %[d]\"\n" + " : [d] \"=rm\" (d)\n" + " [e] \"rm\" (*e));\n" + "}")); +} + TEST_F(FormatTest, GuessLanguageWithChildLines) { EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "#define FOO ({ std::string s; })")); Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -403,8 +403,9 @@ Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) && !CurrentToken->isOneOf(tok::l_brace, tok::r_square) && (!Parent || - Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren, - tok::kw_return, tok::kw_throw) || + (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) || + Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return, + tok::kw_throw) || Parent->isUnaryOperator() || // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen. Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) || @@ -1960,6 +1961,7 @@ Line.First->SpacesRequiredBefore = 1; Line.First->CanBreakBefore = Line.First->MustBreakBefore; + printDebugInfo(Line); } // This function heuristically determines whether 'Current' starts the name of a Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -12753,6 +12753,15 @@ guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);")); } +TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) { + EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", + "void f() {\n" + " asm (\"mov %[e], %[d]\"\n" + " : [d] \"=rm\" (d)\n" + " [e] \"rm\" (*e));\n" + "}")); +} + TEST_F(FormatTest, GuessLanguageWithChildLines) { EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "#define FOO ({ std::string s; })")); Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -403,8 +403,9 @@ Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) && !CurrentToken->isOneOf(tok::l_brace, tok::r_square) && (!Parent || - Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren, - tok::kw_return, tok::kw_throw) || + (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) || + Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return, + tok::kw_throw) || Parent->isUnaryOperator() || // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen. Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) || @@ -1960,6 +1961,7 @@ Line.First->SpacesRequiredBefore = 1; Line.First->CanBreakBefore = Line.First->MustBreakBefore; + printDebugInfo(Line); } // This function heuristically determines whether 'Current' starts the name of a ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54111: [clang-format] Do not threat the asm clobber [ as ObjCExpr
krasimir updated this revision to Diff 172602. krasimir added a comment. - Clean-up Repository: rC Clang https://reviews.llvm.org/D54111 Files: lib/Format/TokenAnnotator.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -12753,6 +12753,15 @@ guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);")); } +TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) { + EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", + "void f() {\n" + " asm (\"mov %[e], %[d]\"\n" + " : [d] \"=rm\" (d)\n" + " [e] \"rm\" (*e));\n" + "}")); +} + TEST_F(FormatTest, GuessLanguageWithChildLines) { EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "#define FOO ({ std::string s; })")); Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -403,8 +403,9 @@ Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) && !CurrentToken->isOneOf(tok::l_brace, tok::r_square) && (!Parent || - Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren, - tok::kw_return, tok::kw_throw) || + (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) || + Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return, + tok::kw_throw) || Parent->isUnaryOperator() || // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen. Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) || Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -12753,6 +12753,15 @@ guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);")); } +TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) { + EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", + "void f() {\n" + " asm (\"mov %[e], %[d]\"\n" + " : [d] \"=rm\" (d)\n" + " [e] \"rm\" (*e));\n" + "}")); +} + TEST_F(FormatTest, GuessLanguageWithChildLines) { EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "#define FOO ({ std::string s; })")); Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -403,8 +403,9 @@ Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) && !CurrentToken->isOneOf(tok::l_brace, tok::r_square) && (!Parent || - Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren, - tok::kw_return, tok::kw_throw) || + (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) || + Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return, + tok::kw_throw) || Parent->isUnaryOperator() || // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen. Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) || ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54112: [Driver] Delete redundant -Bdynamic for libc++ on Fuchsia
MaskRay created this revision. MaskRay added reviewers: phosek, EricWF, mcgrathr. Herald added a subscriber: cfe-commits. The surrounding --push-state saves the "-Bdynamic" state across ld.bfd, gold and lld. lld saves the least states, but the intersection of these linkers is --as-needed -Bdynamic --whole-archive ld/ldlang.h: lang_input_statement_flags::dynamic gold/options.h: Position_dependent_options::copy_from_options lld/ELF/Driver.cpp: Stack.emplace_back(Config->AsNeeded, Config->Static, InWholeArchive); Repository: rC Clang https://reviews.llvm.org/D54112 Files: lib/Driver/ToolChains/Fuchsia.cpp test/Driver/fuchsia.cpp Index: test/Driver/fuchsia.cpp === --- test/Driver/fuchsia.cpp +++ test/Driver/fuchsia.cpp @@ -39,7 +39,6 @@ // CHECK-STATIC: "--as-needed" // CHECK-STATIC: "-Bstatic" // CHECK-STATIC: "-lc++" -// CHECK-STATIC: "-Bdynamic" // CHECK-STATIC: "-lm" // CHECK-STATIC: "--pop-state" // CHECK-STATIC: "-lc" Index: lib/Driver/ToolChains/Fuchsia.cpp === --- lib/Driver/ToolChains/Fuchsia.cpp +++ lib/Driver/ToolChains/Fuchsia.cpp @@ -127,8 +127,6 @@ if (OnlyLibstdcxxStatic) CmdArgs.push_back("-Bstatic"); ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs); -if (OnlyLibstdcxxStatic) - CmdArgs.push_back("-Bdynamic"); CmdArgs.push_back("-lm"); CmdArgs.push_back("--pop-state"); } Index: test/Driver/fuchsia.cpp === --- test/Driver/fuchsia.cpp +++ test/Driver/fuchsia.cpp @@ -39,7 +39,6 @@ // CHECK-STATIC: "--as-needed" // CHECK-STATIC: "-Bstatic" // CHECK-STATIC: "-lc++" -// CHECK-STATIC: "-Bdynamic" // CHECK-STATIC: "-lm" // CHECK-STATIC: "--pop-state" // CHECK-STATIC: "-lc" Index: lib/Driver/ToolChains/Fuchsia.cpp === --- lib/Driver/ToolChains/Fuchsia.cpp +++ lib/Driver/ToolChains/Fuchsia.cpp @@ -127,8 +127,6 @@ if (OnlyLibstdcxxStatic) CmdArgs.push_back("-Bstatic"); ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs); -if (OnlyLibstdcxxStatic) - CmdArgs.push_back("-Bdynamic"); CmdArgs.push_back("-lm"); CmdArgs.push_back("--pop-state"); } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53223: AMDGPU: Add sram-ecc feature options
t-tye accepted this revision. t-tye added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D53223 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54013: [analyzer] MallocChecker: Avoid redundant transitions.
Szelethus added a comment. Please reupload with full context. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2369 ProgramStateRef state = C.getState(); - RegionStateTy RS = state->get(); + RegionStateTy OldRS = state->get(); RegionStateTy::Factory &F = state->get_context(); We are acquiring an object of type `ImmutableMap`, modify it, and put it back into `state`? Can't we just modify it through `ProgramState`'s interface directly? Repository: rC Clang https://reviews.llvm.org/D54013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54112: [Driver] Delete redundant -Bdynamic for libc++ on Fuchsia
MaskRay added a comment. I'm unclear if you also want as-needed `-lm` or if you accept static `-lm` Repository: rC Clang https://reviews.llvm.org/D54112 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54111: [clang-format] Do not threat the asm clobber [ as ObjCExpr
benhamilton accepted this revision. benhamilton added a comment. This revision is now accepted and ready to land. In the diff description, please fix the typo: `Do not threat the asm clobber` -> `Do not treat the asm clobber` Comment at: unittests/Format/FormatTest.cpp:12756-12763 +TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) { + EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", + "void f() {\n" + " asm (\"mov %[e], %[d]\"\n" + " : [d] \"=rm\" (d)\n" + " [e] \"rm\" (*e));\n" + "}")); Worth a second test with `asm volatile (...)` ? Repository: rC Clang https://reviews.llvm.org/D54111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346146 - [AST] Get aliased type info from an aliased TemplateSpecialization.
Author: mattd Date: Mon Nov 5 09:25:26 2018 New Revision: 346146 URL: http://llvm.org/viewvc/llvm-project?rev=346146&view=rev Log: [AST] Get aliased type info from an aliased TemplateSpecialization. Summary: Previously the TemplateSpecialization instance for 'template_alias', in the example below, returned the type info of the canonical type (int). This ignored the type alias if the template type happen to be aliased. Before this patch, the assert would trigger with an alignment of 4: ``` typedef int __attribute__(( aligned( 16 ) )) aligned_int; template < typename > using template_alias = aligned_int; static_assert( alignof( template_alias) == 16, "" ); ``` This patch checks if the TemplateSpecialization type has an alias, and if so will return the type information for the aliased type, else the canonical type's info is returned (original behavior). I believe that this is the desired behavior. Reviewers: aaron.ballman, rjmccall Reviewed By: rjmccall Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D54048 Modified: cfe/trunk/include/clang/AST/Type.h cfe/trunk/test/SemaCXX/alignof.cpp Modified: cfe/trunk/include/clang/AST/Type.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=346146&r1=346145&r2=346146&view=diff == --- cfe/trunk/include/clang/AST/Type.h (original) +++ cfe/trunk/include/clang/AST/Type.h Mon Nov 5 09:25:26 2018 @@ -4901,7 +4901,9 @@ public: return !isDependentType() || isCurrentInstantiation() || isTypeAlias(); } - QualType desugar() const { return getCanonicalTypeInternal(); } + QualType desugar() const { +return isTypeAlias() ? getAliasedType() : getCanonicalTypeInternal(); + } void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Ctx) { Profile(ID, Template, template_arguments(), Ctx); Modified: cfe/trunk/test/SemaCXX/alignof.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/alignof.cpp?rev=346146&r1=346145&r2=346146&view=diff == --- cfe/trunk/test/SemaCXX/alignof.cpp (original) +++ cfe/trunk/test/SemaCXX/alignof.cpp Mon Nov 5 09:25:26 2018 @@ -97,3 +97,8 @@ struct S { typedef __attribute__((aligned(N))) int Field[sizeof(N)]; // expected-error {{requested alignment is dependent but declaration is not dependent}} }; } + +typedef int __attribute__((aligned(16))) aligned_int; +template +using template_alias = aligned_int; +static_assert(alignof(template_alias) == 16, "Expected alignment of 16" ); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54048: [AST] Get aliased type info from an aliased TemplateSpecialization.
This revision was automatically updated to reflect the committed changes. Closed by commit rC346146: [AST] Get aliased type info from an aliased TemplateSpecialization. (authored by mattd, committed by ). Repository: rC Clang https://reviews.llvm.org/D54048 Files: include/clang/AST/Type.h test/SemaCXX/alignof.cpp Index: include/clang/AST/Type.h === --- include/clang/AST/Type.h +++ include/clang/AST/Type.h @@ -4901,7 +4901,9 @@ return !isDependentType() || isCurrentInstantiation() || isTypeAlias(); } - QualType desugar() const { return getCanonicalTypeInternal(); } + QualType desugar() const { +return isTypeAlias() ? getAliasedType() : getCanonicalTypeInternal(); + } void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Ctx) { Profile(ID, Template, template_arguments(), Ctx); Index: test/SemaCXX/alignof.cpp === --- test/SemaCXX/alignof.cpp +++ test/SemaCXX/alignof.cpp @@ -97,3 +97,8 @@ typedef __attribute__((aligned(N))) int Field[sizeof(N)]; // expected-error {{requested alignment is dependent but declaration is not dependent}} }; } + +typedef int __attribute__((aligned(16))) aligned_int; +template +using template_alias = aligned_int; +static_assert(alignof(template_alias) == 16, "Expected alignment of 16" ); Index: include/clang/AST/Type.h === --- include/clang/AST/Type.h +++ include/clang/AST/Type.h @@ -4901,7 +4901,9 @@ return !isDependentType() || isCurrentInstantiation() || isTypeAlias(); } - QualType desugar() const { return getCanonicalTypeInternal(); } + QualType desugar() const { +return isTypeAlias() ? getAliasedType() : getCanonicalTypeInternal(); + } void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Ctx) { Profile(ID, Template, template_arguments(), Ctx); Index: test/SemaCXX/alignof.cpp === --- test/SemaCXX/alignof.cpp +++ test/SemaCXX/alignof.cpp @@ -97,3 +97,8 @@ typedef __attribute__((aligned(N))) int Field[sizeof(N)]; // expected-error {{requested alignment is dependent but declaration is not dependent}} }; } + +typedef int __attribute__((aligned(16))) aligned_int; +template +using template_alias = aligned_int; +static_assert(alignof(template_alias) == 16, "Expected alignment of 16" ); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53212: inhereit LLVM_ENABLE_LIBXML2
modocache accepted this revision. modocache added a comment. This revision is now accepted and ready to land. Sorry to have let this languish! LGTM. Repository: rC Clang https://reviews.llvm.org/D53212 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53919: [X86] Don't allow illegal vector types to return by direct value on x86-64.
echristo added a comment. In https://reviews.llvm.org/D53919#1282994, @hjl.tools wrote: > In https://reviews.llvm.org/D53919#1282952, @efriedma wrote: > > > With both 3.3 and trunk (I don't have a 7.0 handy; I can build it if it > > would be helpful): > > > Please try clang 2.6 on both testcases. From the releases: 23 Oct 2009 2.6 ... why would you care about a 9 year old version of clang? Repository: rC Clang https://reviews.llvm.org/D53919 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53919: [X86] Don't allow illegal vector types to return by direct value on x86-64.
hjl.tools added a comment. In https://reviews.llvm.org/D53919#1287510, @echristo wrote: > In https://reviews.llvm.org/D53919#1282994, @hjl.tools wrote: > > > In https://reviews.llvm.org/D53919#1282952, @efriedma wrote: > > > > > With both 3.3 and trunk (I don't have a 7.0 handy; I can build it if it > > > would be helpful): > > > > > > Please try clang 2.6 on both testcases. > > > From the releases: > > 23 Oct 2009 2.6 > > ... why would you care about a 9 year old version of clang? It is about ABI consistency. When AVX isn't enabled, the old and compilers should use the same calling convention. Repository: rC Clang https://reviews.llvm.org/D53919 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.
echristo added a comment. In https://reviews.llvm.org/D52296#1285328, @grimar wrote: > In https://reviews.llvm.org/D52296#1284130, @probinson wrote: > > > In https://reviews.llvm.org/D52296#1283691, @grimar wrote: > > > > > Nice :) > > > So seems the last unresolved question left is the naming of the new > > > option? > > > If we do not want to go with `-gsingle-file-split-dwarf` then what it > > > should be? > > > > > > What about `-fdebug-fission` as an alias for `-gsplit-dwarf`. > > > And `-fsingle-file-debug-fission` for the new option? > > > > > > Or, may be: > > > > > > `-fdebug-fission` for the new option and then > > > `-fdebug-fission -fdebug-split-dwo` would work together as > > > `-gsplit-dwarf`? > > > > > > Only DWARF supports this, correct? > > > I am not aware of any kind of debug information splitting except DWARF > splitting. > > > So I would suggest: `-fdwarf-fission[={split,single}]` where the parameter > > defaults to `split`. Is there any particular value in having two separate > > options? > > Probably not. `-fdwarf-fission[={split,single}]` sounds good for me. Still not sure I understand the point, but this option and the alias sounds good to me. Thanks. -eric https://reviews.llvm.org/D52296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D53919: [X86] Don't allow illegal vector types to return by direct value on x86-64.
On Mon, Nov 5, 2018 at 9:45 AM H.J Lu via Phabricator < revi...@reviews.llvm.org> wrote: > hjl.tools added a comment. > > In https://reviews.llvm.org/D53919#1287510, @echristo wrote: > > > In https://reviews.llvm.org/D53919#1282994, @hjl.tools wrote: > > > > > In https://reviews.llvm.org/D53919#1282952, @efriedma wrote: > > > > > > > With both 3.3 and trunk (I don't have a 7.0 handy; I can build it if > it would be helpful): > > > > > > > > > Please try clang 2.6 on both testcases. > > > > > > From the releases: > > > > 23 Oct 2009 2.6 > > > > ... why would you care about a 9 year old version of clang? > > > It is about ABI consistency. When AVX isn't enabled, the old and > compilers should > use the same calling convention. > Seems like 3.3 to present should be sufficient. -eric ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53457: clang-cl: Add "/clang:" pass-through arg support.
neerajksingh updated this revision to Diff 172615. neerajksingh added a comment. Make it clear in the documentation that the /clang flags are added to the end. https://reviews.llvm.org/D53457 Files: docs/UsersManual.rst include/clang/Driver/CLCompatOptions.td include/clang/Driver/Driver.h lib/Driver/Driver.cpp test/Driver/cl-options.c Index: test/Driver/cl-options.c === --- test/Driver/cl-options.c +++ test/Driver/cl-options.c @@ -614,5 +614,17 @@ // RUN: --version \ // RUN: -Werror /Zs -- %s 2>&1 +// Accept clang options under the /clang: flag. + +// RUN: %clang_cl -O2 -### -- %s 2>&1 | FileCheck -check-prefix=NOCLANG %s +// NOCLANG: "--dependent-lib=libcmt" +// NOCLANG-SAME: "-vectorize-slp" +// NOCLANG-NOT: "--dependent-lib=msvcrt" + +// RUN: %clang_cl -O2 -MD /clang:-fno-slp-vectorize /clang:-MD /clang:-MF /clang:my_dependency_file.dep -### -- %s 2>&1 | FileCheck -check-prefix=CLANG %s +// CLANG: "--dependent-lib=msvcrt" +// CLANG-SAME: "-dependency-file" "my_dependency_file.dep" +// CLANG-NOT: "--dependent-lib=libcmt" +// CLANG-NOT: "-vectorize-slp" void f() { } Index: lib/Driver/Driver.cpp === --- lib/Driver/Driver.cpp +++ lib/Driver/Driver.cpp @@ -166,14 +166,15 @@ } InputArgList Driver::ParseArgStrings(ArrayRef ArgStrings, + bool IsClCompatMode, bool &ContainsError) { llvm::PrettyStackTraceString CrashInfo("Command line argument parsing"); ContainsError = false; unsigned IncludedFlagsBitmask; unsigned ExcludedFlagsBitmask; std::tie(IncludedFlagsBitmask, ExcludedFlagsBitmask) = - getIncludeExcludeOptionFlagMasks(); + getIncludeExcludeOptionFlagMasks(IsClCompatMode); unsigned MissingArgIndex, MissingArgCount; InputArgList Args = @@ -730,7 +731,7 @@ ConfigFile = CfgFileName.str(); bool ContainErrors; CfgOptions = llvm::make_unique( - ParseArgStrings(NewCfgArgs, ContainErrors)); + ParseArgStrings(NewCfgArgs, IsCLMode(), ContainErrors)); if (ContainErrors) { CfgOptions.reset(); return true; @@ -924,7 +925,7 @@ // Arguments specified in command line. bool ContainsError; CLOptions = llvm::make_unique( - ParseArgStrings(ArgList.slice(1), ContainsError)); + ParseArgStrings(ArgList.slice(1), IsCLMode(), ContainsError)); // Try parsing configuration file. if (!ContainsError) @@ -934,21 +935,47 @@ // All arguments, from both config file and command line. InputArgList Args = std::move(HasConfigFile ? std::move(*CfgOptions) : std::move(*CLOptions)); - if (HasConfigFile) -for (auto *Opt : *CLOptions) { - if (Opt->getOption().matches(options::OPT_config)) -continue; + + auto appendOneArg = [&Args](const Arg *Opt, const Arg *BaseArg) { unsigned Index = Args.MakeIndex(Opt->getSpelling()); - const Arg *BaseArg = &Opt->getBaseArg(); - if (BaseArg == Opt) -BaseArg = nullptr; Arg *Copy = new llvm::opt::Arg(Opt->getOption(), Opt->getSpelling(), Index, BaseArg); Copy->getValues() = Opt->getValues(); if (Opt->isClaimed()) Copy->claim(); Args.append(Copy); + }; + + if (HasConfigFile) +for (auto *Opt : *CLOptions) { + if (Opt->getOption().matches(options::OPT_config)) +continue; + const Arg *BaseArg = &Opt->getBaseArg(); + if (BaseArg == Opt) +BaseArg = nullptr; + appendOneArg(Opt, BaseArg); +} + + // In CL mode, look for any pass-through arguments + if (IsCLMode() && !ContainsError) { +SmallVector CLModePassThroughArgList; +for (const auto *A : Args.filtered(options::OPT__SLASH_clang)) { + A->claim(); + CLModePassThroughArgList.push_back(A->getValue()); +} + +if (!CLModePassThroughArgList.empty()) { + // Parse any pass through args using default clang processing rather + // than clang-cl processing. + auto CLModePassThroughOptions = llvm::make_unique( + ParseArgStrings(CLModePassThroughArgList, false, ContainsError)); + + if (!ContainsError) +for (auto *Opt : *CLModePassThroughOptions) { + appendOneArg(Opt, nullptr); +} } + } // FIXME: This stuff needs to go into the Compilation, not the driver. bool CCCPrintPhases; @@ -1452,7 +1479,7 @@ unsigned IncludedFlagsBitmask; unsigned ExcludedFlagsBitmask; std::tie(IncludedFlagsBitmask, ExcludedFlagsBitmask) = - getIncludeExcludeOptionFlagMasks(); + getIncludeExcludeOptionFlagMasks(IsCLMode()); ExcludedFlagsBitmask |= options::NoDriverOption; if (!ShowHidden) @@ -4661,11 +4688,11 @@ return false; } -std::pair Driver::getIncludeExcludeOptionFlagMasks() const { +std::pair Driver::getIncludeExcludeOptionFlagMasks(bool Is
[PATCH] D54013: [analyzer] MallocChecker: Avoid redundant transitions.
george.karpenkov added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2369 ProgramStateRef state = C.getState(); - RegionStateTy RS = state->get(); + RegionStateTy OldRS = state->get(); RegionStateTy::Factory &F = state->get_context(); Szelethus wrote: > We are acquiring an object of type `ImmutableMap`, modify it, and put it back > into `state`? Can't we just modify it through `ProgramState`'s interface > directly? state is immutable, I don't think we can put anything into it. We also can't modify ImmutableMap because, well, it's immutable. Repository: rC Clang https://reviews.llvm.org/D54013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54013: [analyzer] MallocChecker: Avoid redundant transitions.
Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2369 ProgramStateRef state = C.getState(); - RegionStateTy RS = state->get(); + RegionStateTy OldRS = state->get(); RegionStateTy::Factory &F = state->get_context(); george.karpenkov wrote: > Szelethus wrote: > > We are acquiring an object of type `ImmutableMap`, modify it, and put it > > back into `state`? Can't we just modify it through `ProgramState`'s > > interface directly? > state is immutable, I don't think we can put anything into it. > We also can't modify ImmutableMap because, well, it's immutable. Poor choice of words, I admit. What I actually meant is that maybe it would be more straighforward if we didnt create a local varable here. But I'm fine with being in the wrong :) Repository: rC Clang https://reviews.llvm.org/D54013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r345695 - Change "struct" to "class" to avoid warnings
Could you link to/quote the warnings - might be helpful to understanding what's being addressed here On Tue, Oct 30, 2018 at 10:00 PM Bill Wendling via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: void > Date: Tue Oct 30 21:58:34 2018 > New Revision: 345695 > > URL: http://llvm.org/viewvc/llvm-project?rev=345695&view=rev > Log: > Change "struct" to "class" to avoid warnings > > Modified: > cfe/trunk/include/clang/AST/Expr.h > > Modified: cfe/trunk/include/clang/AST/Expr.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=345695&r1=345694&r2=345695&view=diff > > == > --- cfe/trunk/include/clang/AST/Expr.h (original) > +++ cfe/trunk/include/clang/AST/Expr.h Tue Oct 30 21:58:34 2018 > @@ -900,7 +900,8 @@ public: > }; > > /// ConstantExpr - An expression that occurs in a constant context. > -struct ConstantExpr : public FullExpr { > +class ConstantExpr : public FullExpr { > +public: >ConstantExpr(Expr *subexpr) > : FullExpr(ConstantExprClass, subexpr) {} > > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r345591 - [CodeGen] Disable the machine verifier on a ThinLTO test
If ThinLTO doesn't pass the machine verifier - should it maybe be turned off at the thinlto level in general, rather than for this specific test? On Tue, Oct 30, 2018 at 5:20 AM Francis Visoiu Mistrih via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: thegameg > Date: Tue Oct 30 05:18:33 2018 > New Revision: 345591 > > URL: http://llvm.org/viewvc/llvm-project?rev=345591&view=rev > Log: > [CodeGen] Disable the machine verifier on a ThinLTO test > > This allows us to turn the machine verifier on by default on X86. > > Modified: > cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll > > Modified: cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll?rev=345591&r1=345590&r2=345591&view=diff > > == > --- cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll (original) > +++ cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll Tue Oct 30 > 05:18:33 2018 > @@ -6,7 +6,9 @@ > > ; RUN: opt -thinlto-bc -o %t.o %s > > +; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. > PR39436. > ; RUN: llvm-lto2 run -thinlto-distributed-indexes %t.o \ > +; RUN: -verify-machineinstrs=0 \ > ; RUN: -o %t2.index \ > ; RUN: -r=%t.o,test,px \ > ; RUN: -r=%t.o,_ZN1A1nEi,p \ > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52790: [analyzer][PlistMacroExpansion] New flag to convert macro expansions to events
Szelethus added a comment. In https://reviews.llvm.org/D52790#1285039, @NoQ wrote: > Looks great, let's land? I'll probably land it after part 5, in order to ease on rebasing. > Not sure if i already asked - am i understanding correctly that this is a > "poor-man's" support for macro expansions for tools that read plists but do > not support those new plist macro dictionaries yet? Correct, same as `notes-as-events`. > I wonder how expansions of huge macros will look as events. Quite funny, actually :D Think about assert, it's quite a mouthful. I'll post screenshots when I have more time. https://reviews.llvm.org/D52790 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54062: [COFF, ARM64] Implement InterlockedCompareExchange*_* builtins
rnk added inline comments. Comment at: include/clang/Basic/BuiltinsARM.def:270 +TARGET_HEADER_BUILTIN(_InterlockedCompareExchange64_nf, "LLiLLiD*LLiLLi", "nh", "intrin.h", ALL_MS_LANGUAGES, "") +TARGET_HEADER_BUILTIN(_InterlockedCompareExchange64_rel, "LLiLLiD*LLiLLi", "nh", "intrin.h", ALL_MS_LANGUAGES, "") + Given how much duplication there is, I think we need to have some kind of BuiltinsMSVC.def that contains a list of all the interlocked builitin families, like this: ``` INTERLOCKED_BUILTIN(_InterlockedCompareExchange) INTERLOCKED_BUILTIN(_InterlockedOr) INTERLOCKED_BUILTIN(_InterlockedAnd) ``` We'd include this file here, with INTERLOCKED_BUILTIN defined as: ``` #define INTERLOCKED_BUILTIN(Op) \ TARGET_HEADER_BUILTIN(Op##8_acq, "ccD*cc", "nh", "intrin.h", ALL_MS_LANGUAGES, "") \ TARGET_HEADER_BUILTIN(Op##8_nf, "ccD*cc", "nh", "intrin.h", ALL_MS_LANGUAGES, "") \ ... ``` That'll stamp out the enums that we need, and then we can reuse the .def file to reduce duplication in the switch/case below. Every op is basically 16 operations: {8, 16, 32, 64} X {seq_cst, rel, acq, nf} I'm not sure what to do about the ones that overlap with x86. Repository: rC Clang https://reviews.llvm.org/D54062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54013: [analyzer] MallocChecker: Avoid redundant transitions.
NoQ updated this revision to Diff 172629. NoQ added a comment. Re-upload with context. Whoops. https://reviews.llvm.org/D54013 Files: lib/StaticAnalyzer/Checkers/MallocChecker.cpp Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp === --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -2366,20 +2366,23 @@ CheckerContext &C) const { ProgramStateRef state = C.getState(); - RegionStateTy RS = state->get(); + RegionStateTy OldRS = state->get(); RegionStateTy::Factory &F = state->get_context(); + RegionStateTy RS = OldRS; SmallVector Errors; for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; ++I) { if (SymReaper.isDead(I->first)) { if (I->second.isAllocated() || I->second.isAllocatedOfSizeZero()) Errors.push_back(I->first); // Remove the dead symbol from the map. RS = F.remove(RS, I->first); - } } + if (RS == OldRS) +return; + // Cleanup the Realloc Pairs Map. ReallocPairsTy RP = state->get(); for (ReallocPairsTy::iterator I = RP.begin(), E = RP.end(); I != E; ++I) { Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp === --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -2366,20 +2366,23 @@ CheckerContext &C) const { ProgramStateRef state = C.getState(); - RegionStateTy RS = state->get(); + RegionStateTy OldRS = state->get(); RegionStateTy::Factory &F = state->get_context(); + RegionStateTy RS = OldRS; SmallVector Errors; for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; ++I) { if (SymReaper.isDead(I->first)) { if (I->second.isAllocated() || I->second.isAllocatedOfSizeZero()) Errors.push_back(I->first); // Remove the dead symbol from the map. RS = F.remove(RS, I->first); - } } + if (RS == OldRS) +return; + // Cleanup the Realloc Pairs Map. ReallocPairsTy RP = state->get(); for (ReallocPairsTy::iterator I = RP.begin(), E = RP.end(); I != E; ++I) { ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54013: [analyzer] NFC: MallocChecker: Avoid redundant transitions.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2369 ProgramStateRef state = C.getState(); - RegionStateTy RS = state->get(); + RegionStateTy OldRS = state->get(); RegionStateTy::Factory &F = state->get_context(); Szelethus wrote: > george.karpenkov wrote: > > Szelethus wrote: > > > We are acquiring an object of type `ImmutableMap`, modify it, and put it > > > back into `state`? Can't we just modify it through `ProgramState`'s > > > interface directly? > > state is immutable, I don't think we can put anything into it. > > We also can't modify ImmutableMap because, well, it's immutable. > Poor choice of words, I admit. What I actually meant is that maybe it would > be more straighforward if we didnt create a local varable here. But I'm fine > with being in the wrong :) Manipulating maps directly is slightly cheaper because for every operation you only create a new map but not a new state. I have no indication that this optimization is worth the effort, but i also have no indication that pessimizing it back is worth the effort. https://reviews.llvm.org/D54013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:142 + if (LoopVar->getType() != LoopIncrement->getType()) +return; // We matched the loop variable incorrectly + JonasToth wrote: > Does this try to ensure a precondition? Then it should become an assertion > instead. > Please adjust the comment like above (punctuation, position) It's not an assumed precondition. This `if` handles the case when LoopVarMatcher is not fitted with the actual loop variable. That's why the IncrementMatcher is there so we can check whether we found the loop variable. See voidForLoopReverseCond() test case which hits this `if` branch. I did not find a solution to handle this check inside the matcher. Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10 + + .. code-block:: c++ + JonasToth wrote: > the `.. code-block:: c++` is usually not indended, only the code itself. Hmm, I copied this from somewhere. It might be a good idea to make this consistent across all the *.rst files. See bugprone-suspicious-semicolon.rst or bugprone-use-after-move.rst for example. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:45 + + // We need to catch only those comparisons which contain any integer cast + StatementMatcher LoopVarConversionMatcher = JonasToth wrote: > missing full stop. Sorry, I forgot the punctuation what you've already mentioned. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54062: [COFF, ARM64] Implement InterlockedCompareExchange*_* builtins
mgrang added inline comments. Comment at: include/clang/Basic/BuiltinsARM.def:270 +TARGET_HEADER_BUILTIN(_InterlockedCompareExchange64_nf, "LLiLLiD*LLiLLi", "nh", "intrin.h", ALL_MS_LANGUAGES, "") +TARGET_HEADER_BUILTIN(_InterlockedCompareExchange64_rel, "LLiLLiD*LLiLLi", "nh", "intrin.h", ALL_MS_LANGUAGES, "") + rnk wrote: > Given how much duplication there is, I think we need to have some kind of > BuiltinsMSVC.def that contains a list of all the interlocked builitin > families, like this: > ``` > INTERLOCKED_BUILTIN(_InterlockedCompareExchange) > INTERLOCKED_BUILTIN(_InterlockedOr) > INTERLOCKED_BUILTIN(_InterlockedAnd) > ``` > We'd include this file here, with INTERLOCKED_BUILTIN defined as: > ``` > #define INTERLOCKED_BUILTIN(Op) \ > TARGET_HEADER_BUILTIN(Op##8_acq, "ccD*cc", "nh", "intrin.h", > ALL_MS_LANGUAGES, "") \ > TARGET_HEADER_BUILTIN(Op##8_nf, "ccD*cc", "nh", "intrin.h", > ALL_MS_LANGUAGES, "") \ > ... > ``` > That'll stamp out the enums that we need, and then we can reuse the .def file > to reduce duplication in the switch/case below. > > Every op is basically 16 operations: {8, 16, 32, 64} X {seq_cst, rel, acq, nf} > > I'm not sure what to do about the ones that overlap with x86. Thanks Reid. Yes, I think we can certainly cleanup further. That being said, can we get these patches committed first and then cleanup in a follow-up patch? Repository: rC Clang https://reviews.llvm.org/D54062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas updated this revision to Diff 172635. ztamas added a comment. - Add a range-based loop test case - Restructure test cases a bit - Fix-up comments, position, punctuation Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53974 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp clang-tidy/bugprone/TooSmallLoopVariableCheck.h docs/ReleaseNotes.rst docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst docs/clang-tidy/checks/list.rst test/clang-tidy/bugprone-too-small-loop-variable.cpp Index: test/clang-tidy/bugprone-too-small-loop-variable.cpp === --- /dev/null +++ test/clang-tidy/bugprone-too-small-loop-variable.cpp @@ -0,0 +1,227 @@ +// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t + +long size() { return 294967296l; } + + +/// Test cases caught by bugprone-too-small-loop-variable. + +void voidBadForLoop() { + for (int i = 0; i < size(); ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable] + } +} + +void voidBadForLoop2() { + for (int i = 0; i < size() + 10; ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable] + } +} + +void voidBadForLoop3() { + for (int i = 0; i <= size() - 1; ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable] + } +} + +void voidBadForLoop4() { + for (int i = 0; size() > i; ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable] + } +} + +void voidBadForLoop5() { + for (int i = 0; size() - 1 >= i; ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable] + } +} + +void voidBadForLoop6() { + int i = 0; + for (; i < size(); ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than terminating condition 'long' [bugprone-too-small-loop-variable] + } +} + +void voidForLoopUnsignedCond() { + unsigned size = 3147483647; + for (int i = 0; i < size; ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than terminating condition 'unsigned int' [bugprone-too-small-loop-variable] + } +} + +// Loop's terminating condition has a template dependent value. +template +void doSomething() { + for (short i = 0; i < size; ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than terminating condition 'long' [bugprone-too-small-loop-variable] + } +} + +// Loop's terminating condition has a template dependent type. +template +void doSomething() { + for (T i = 0; i < size(); ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than terminating condition 'long' [bugprone-too-small-loop-variable] + } +} + +void voidForLoopInstantiation() { + // This line does not trigger the warning. + doSomething(); + // This one triggers the warning. + doSomething(); +} + + +/// Test cases where we should not warn. + +// A simple use case when both expressions have the same type. +void voidGoodForLoop() { + for (long i = 0; i < size(); ++i) { // no warning + } +} + +// Other use case where both expressions have the same type, +// but short expressions are converted to int by the compare operator. +void voidGoodForLoop2() { + short loopCond = 10; + for (short i = 0; i < loopCond; ++i) { // no warning + } +} + +// Because of the integer literal, the loop condition is int, but we suppress the warning here. +void voidForLoopShortPlusLiteral() { + short size = 3; + for (short i = 0; i <= (size - 1); ++i) { // no warning + } +} + +// Addition of two short variables results in an int value, but we suppress this to avoid false positives. +void voidForLoopShortPlusShort() { + short size = 256; + short increment = 14; + for (short i = 0; i < size + increment; ++i) { // no warning + } +} + +// In this test case we have different integer types, but here the loop variable has the bigger type. +// The terminating condition is cast implicitly, not the loop variable. +void voidForLoopCondImplicitCast() { + short start = 256; + short end = 14; + for (int i = start; i >= end; --i) { // no warning + } +} + +// Range based loop and other iterator based loops are ignored by this check. +void voidRangeBasedForLoop() { + int array[] =
[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.
george.karpenkov added a comment. @lebedev.ri @erichkeane The test fails for me on macOS whenever asan and ubsan are both enabled. The failure is stack overflow at stack frame 943 (? maybe asan usage enforces lower stack size?) Repository: rC Clang https://reviews.llvm.org/D50050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53928: Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions
steleman updated this revision to Diff 172638. steleman added a comment. - changed the -fveclib= argument value to 'sleefgnuabi'. - added atan2 and pow. - spreadsheet with comparison between libm and sleef is here: https://docs.google.com/spreadsheets/d/1lcpESCnuzEoTl_XHBqE9FLL0tXJB_tZGR8yciCx1yjg/edit?usp=sharing - comprehensive test case in C with timings is here: https://drive.google.com/open?id=1PGKRUdL29_ANoYebOo3Q59syhKp_mNSj https://reviews.llvm.org/D53928 Files: include/clang/Basic/Builtins.def include/clang/Driver/Options.td include/clang/Frontend/CodeGenOptions.h lib/CodeGen/BackendUtil.cpp lib/CodeGen/CGBuiltin.cpp lib/Frontend/CompilerInvocation.cpp Index: include/clang/Frontend/CodeGenOptions.h === --- include/clang/Frontend/CodeGenOptions.h +++ include/clang/Frontend/CodeGenOptions.h @@ -54,7 +54,8 @@ enum VectorLibrary { NoLibrary, // Don't use any vector library. Accelerate, // Use the Accelerate framework. -SVML// Intel short vector math library. +SVML, // Intel short vector math library. +SLEEFGNUABI // SLEEF - SIMD Library for Evaluating Elementary Functions. }; Index: include/clang/Driver/Options.td === --- include/clang/Driver/Options.td +++ include/clang/Driver/Options.td @@ -1388,7 +1388,7 @@ Group, Flags<[CC1Option]>, HelpText<"Disables an experimental new pass manager in LLVM.">; def fveclib : Joined<["-"], "fveclib=">, Group, Flags<[CC1Option]>, -HelpText<"Use the given vector functions library">, Values<"Accelerate,SVML,none">; +HelpText<"Use the given vector functions library">, Values<"Accelerate,SVML,sleefgnuabi,none">; def fno_lax_vector_conversions : Flag<["-"], "fno-lax-vector-conversions">, Group, HelpText<"Disallow implicit conversions between vectors with a different number of elements or different element types">, Flags<[CC1Option]>; def fno_merge_all_constants : Flag<["-"], "fno-merge-all-constants">, Group, Index: include/clang/Basic/Builtins.def === --- include/clang/Basic/Builtins.def +++ include/clang/Basic/Builtins.def @@ -196,6 +196,9 @@ BUILTIN(__builtin_exp2 , "dd" , "Fne") BUILTIN(__builtin_exp2f, "ff" , "Fne") BUILTIN(__builtin_exp2l, "LdLd", "Fne") +BUILTIN(__builtin_exp10 , "dd" , "Fne") +BUILTIN(__builtin_exp10f, "ff" , "Fne") +BUILTIN(__builtin_exp10l, "LdLd", "Fne") BUILTIN(__builtin_expm1 , "dd", "Fne") BUILTIN(__builtin_expm1f, "ff", "Fne") BUILTIN(__builtin_expm1l, "LdLd", "Fne") @@ -1133,6 +1136,10 @@ LIBBUILTIN(exp2f, "ff", "fne", "math.h", ALL_LANGUAGES) LIBBUILTIN(exp2l, "LdLd", "fne", "math.h", ALL_LANGUAGES) +LIBBUILTIN(exp10, "dd", "fne", "math.h", ALL_LANGUAGES) +LIBBUILTIN(exp10f, "ff", "fne", "math.h", ALL_LANGUAGES) +LIBBUILTIN(exp10l, "LdLd", "fne", "math.h", ALL_LANGUAGES) + LIBBUILTIN(expm1, "dd", "fne", "math.h", ALL_LANGUAGES) LIBBUILTIN(expm1f, "ff", "fne", "math.h", ALL_LANGUAGES) LIBBUILTIN(expm1l, "LdLd", "fne", "math.h", ALL_LANGUAGES) @@ -1370,10 +1377,6 @@ LIBBUILTIN(__tanpi, "dd", "fne", "math.h", ALL_LANGUAGES) LIBBUILTIN(__tanpif, "ff", "fne", "math.h", ALL_LANGUAGES) -// Similarly, __exp10 is OS X only -LIBBUILTIN(__exp10, "dd", "fne", "math.h", ALL_LANGUAGES) -LIBBUILTIN(__exp10f, "ff", "fne", "math.h", ALL_LANGUAGES) - // Blocks runtime Builtin math library functions LIBBUILTIN(_Block_object_assign, "vv*vC*iC", "f", "Blocks.h", ALL_LANGUAGES) LIBBUILTIN(_Block_object_dispose, "vvC*iC", "f", "Blocks.h", ALL_LANGUAGES) Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -558,6 +558,8 @@ Opts.setVecLib(CodeGenOptions::Accelerate); else if (Name == "SVML") Opts.setVecLib(CodeGenOptions::SVML); +else if (Name == "sleefgnuabi") + Opts.setVecLib(CodeGenOptions::SLEEFGNUABI); else if (Name == "none") Opts.setVecLib(CodeGenOptions::NoLibrary); else Index: lib/CodeGen/BackendUtil.cpp === --- lib/CodeGen/BackendUtil.cpp +++ lib/CodeGen/BackendUtil.cpp @@ -344,6 +344,9 @@ case CodeGenOptions::SVML: TLII->addVectorizableFunctionsFromVecLib(TargetLibraryInfoImpl::SVML); break; + case CodeGenOptions::SLEEFGNUABI: +TLII->addVectorizableFunctionsFromVecLib(TargetLibraryInfoImpl::SLEEFGNUABI); +break; default: break; } Index: lib/CodeGen/CGBuiltin.cpp === --- lib/CodeGen/CGBuiltin.cpp +++ lib/CodeGen/CGBuiltin.cpp @@ -1333,14 +1333,78 @@ case Builtin::BI__builtin_copysignf128: return RValue::get(emitBinaryBuiltin(*this, E, Intrinsic::copysign)); +case Builtin::BIacos: +case Builti