[PATCH] D45449: [CUDA] Document recent changes
Hahnfeld added a comment. In https://reviews.llvm.org/D45449#1062642, @tra wrote: > We could use specific instructions for what exactly one needs to do in order > to use relocatable device code in practice. Could be a separate patch. Maybe I should add an example to https://llvm.org/docs/CompileCudaWithLLVM.html? Repository: rC Clang https://reviews.llvm.org/D45449 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.
courbet added a comment. In https://reviews.llvm.org/D38455#1061681, @JonasToth wrote: > Could you please add some tests that include user defined literals and there > interaction with other literals. We should catch narrowing conversions from > them, too. User defined literals do not have this issue: the only accepted signatures are with `long double` or `unsigned long long` parameters. Narrowing cannot happen because `XYZ_ud` actually means `operator""(XYZull)` (same for floats). (see http://en.cppreference.com/w/cpp/language/user_literal). I've added a test with a narrowing on the return value. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.
courbet added a comment. In https://reviews.llvm.org/D38455#1061926, @pfultz2 wrote: > This seems like it would be nice to have in `bugprone-*` as well. Sure. Is it OK to add a dependency from `clang-tidy/bugprone/BugproneTidyModule.cpp` to stuff in `clang-tidy/cpp-coreguidelines` ? Is there an existing example of this ? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.
courbet updated this revision to Diff 141800.
courbet added a comment.
- Simplify matcher expression
- Add other binary operators
- Add more tests.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38455
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+
+float ceil(float);
+namespace std {
+double ceil(double);
+long double floor(long double);
+} // namespace std
+
+namespace floats {
+
+struct ConvertsToFloat {
+ operator float() const { return 0.5; }
+};
+
+float operator "" _Pa(unsigned long long);
+
+void not_ok(double d) {
+ int i = 0;
+ i = d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i = 0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i = static_cast(d);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i = ConvertsToFloat();
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i = 15_Pa;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+void not_ok_binary_ops(double d) {
+ int i = 0;
+ i += 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += 0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // We warn on the following even though it's not dangerous because there is no
+ // reason to use a double literal here.
+ // TODO(courbet): Provide an automatic fix.
+ i += 2.0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += 2.0f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+
+ i *= 0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i /= 0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+void ok(double d) {
+ int i = 0;
+ i = 1;
+ i = static_cast(0.5);
+ i = static_cast(d);
+ i = std::ceil(0.5);
+ i = ::std::floor(0.5);
+ {
+using std::ceil;
+i = ceil(0.5f);
+ }
+ i = ceil(0.5f);
+}
+
+void ok_binary_ops(double d) {
+ int i = 0;
+ i += 1;
+ i += static_cast(0.5);
+ i += static_cast(d);
+ i += std::ceil(0.5);
+ i += ::std::floor(0.5);
+ {
+using std::ceil;
+i += ceil(0.5f);
+ }
+ i += ceil(0.5f);
+}
+
+} // namespace floats
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -76,6 +76,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator)
cppcoreguidelines-interfaces-global-init
+ cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - cppcoreguidelines-narrowing-conversions
+
+cppcoreguidelines-narrowing-conversions
+===
+
+Checks for silent narrowing conversions, e.g: ``int i = 0; i += 0.1;``. While
+the issue is obvious in this former example, it might not be so in the
+following: ``void MyClass::f(double d) { int_member_ += d; }``.
+
+This rule is part of the "Expressions and statements" profile of the C++ Core
+Guidelines, corresponding to rul
[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.
JonasToth added a comment. > Sure. Is it OK to add a dependency from > clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in > clang-tidy/cpp-coreguidelines ? Is there an existing example of this ? Take a look in the hicpp module, almost everything there is aliased :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.
courbet updated this revision to Diff 141801.
courbet added a comment.
- Add NarrowingConversionsCheck to bugprone module.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38455
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+
+float ceil(float);
+namespace std {
+double ceil(double);
+long double floor(long double);
+} // namespace std
+
+namespace floats {
+
+struct ConvertsToFloat {
+ operator float() const { return 0.5; }
+};
+
+float operator "" _Pa(unsigned long long);
+
+void not_ok(double d) {
+ int i = 0;
+ i = d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i = 0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i = static_cast(d);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i = ConvertsToFloat();
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i = 15_Pa;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+void not_ok_binary_ops(double d) {
+ int i = 0;
+ i += 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += 0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // We warn on the following even though it's not dangerous because there is no
+ // reason to use a double literal here.
+ // TODO(courbet): Provide an automatic fix.
+ i += 2.0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += 2.0f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+
+ i *= 0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i /= 0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+void ok(double d) {
+ int i = 0;
+ i = 1;
+ i = static_cast(0.5);
+ i = static_cast(d);
+ i = std::ceil(0.5);
+ i = ::std::floor(0.5);
+ {
+using std::ceil;
+i = ceil(0.5f);
+ }
+ i = ceil(0.5f);
+}
+
+void ok_binary_ops(double d) {
+ int i = 0;
+ i += 1;
+ i += static_cast(0.5);
+ i += static_cast(d);
+ i += std::ceil(0.5);
+ i += ::std::floor(0.5);
+ {
+using std::ceil;
+i += ceil(0.5f);
+ }
+ i += ceil(0.5f);
+}
+
+} // namespace floats
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -76,6 +76,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator)
cppcoreguidelines-interfaces-global-init
+ cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - cppcoreguidelines-narrowing-conversions
+
+cppcoreguidelines-narrowing-conversions
+===
+
+Checks for silent narrowing conversions, e.g: ``int i = 0; i += 0.1;``. While
+the issue is obvious in this former example, it might not be so in the
+following: ``void MyClass::f(double d) { int_member_ += d; }``.
+
+This rule is part of the "Expressions and statements"
[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.
courbet added a comment. In https://reviews.llvm.org/D38455#1062718, @JonasToth wrote: > > Sure. Is it OK to add a dependency from > > clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in > > clang-tidy/cpp-coreguidelines ? Is there an existing example of this ? > > Take a look in the hicpp module, almost everything there is aliased :) Thanks for the pointer ! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43667: [clang-doc] Implement a YAML generator
Athosvk added a comment.
I'm a bit late on this, but I'd say that YAML is usually not a 'final' format.
What would be the use-cases for this? And if is meant as an alternative
intermediate format, why not instead of having one big file, have one file per
input file? Just wondering what the particular motivation for that could be
Comment at: clang-doc/generators/YAMLGenerator.cpp:159
+if (!C->Position.empty()) IO.mapRequired("Position", C->Position);
+if (!C->Children.empty()) IO.mapRequired("Children", C->Children);
+ }
You should easily be able to unify these 'empty' checks
https://reviews.llvm.org/D43667
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43667: [clang-doc] Implement a YAML generator
Athosvk added a comment. I'm a bit late on this, but I'd say that YAML is usually not a 'final' format. What would be the use-cases for this? And if is meant as an alternative intermediate format, why not instead of having one big file, have one file per input file? Just wondering what the particular motivation for that could be (Don't mind the previous and inline comment, it was old and I never got to submit it. Differential doesn't let me remove it unfortunately) https://reviews.llvm.org/D43667 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43764: [clang-apply-replacements] Convert tooling::Replacements to tooling::AtomicChange for conflict resolving of changes, code cleanup, and code formatting.
jdemeule added a comment. Ok, I understand the problem. Previously, during the deduplication step, replacements per file where sorted and it is not the case anymore. That means now, clang-apply-replacement is highly dependant of reading file order during error reporting. I mainly see 2 options (I can miss others), rewrite the test (e.g merging yaml file together) or sort the replacements per file to fix the order of application. Can I ask you what solution you prefer? https://reviews.llvm.org/D43764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45478: [clangd] Merge symbols in global-sym-builder on the go
ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, sammccall, klimek.
Herald added subscribers: MaskRay, ioeric, jkorous-apple.
This avoids storing intermediate symbols in memory, most of which are
duplicates.
The resulting .yaml file is ~120MB, while intermediate symbols takes
more than 20GB.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45478
Files:
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
Index: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
===
--- clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
+++ clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
@@ -31,6 +31,7 @@
#include "llvm/Support/Signals.h"
#include "llvm/Support/ThreadPool.h"
#include "llvm/Support/YAMLTraits.h"
+#include
using namespace llvm;
using namespace clang::tooling;
@@ -49,22 +50,47 @@
"not given, such headers will have relative paths."),
llvm::cl::init(""));
+/// Combines occurrences of the same symbols across translation units.
+class SymbolMerger {
+public:
+ void mergeSymbols(const SymbolSlab &Symbols) {
+std::lock_guard Lock(Mut);
+for (const Symbol &Sym : Symbols) {
+ if (const auto *Existing = UniqueSymbols.find(Sym.ID)) {
+Symbol::Details Scratch;
+UniqueSymbols.insert(mergeSymbol(*Existing, Sym, &Scratch));
+ } else {
+UniqueSymbols.insert(Sym);
+ }
+}
+ }
+
+ SymbolSlab build() && {
+std::lock_guard Lock(Mut);
+return std::move(UniqueSymbols).build();
+ }
+
+private:
+ std::mutex Mut;
+ SymbolSlab::Builder UniqueSymbols;
+};
+
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
- SymbolIndexActionFactory(tooling::ExecutionContext *Ctx) : Ctx(Ctx) {}
+ SymbolIndexActionFactory(SymbolMerger &Merger) : Merger(Merger) {}
clang::FrontendAction *create() override {
// Wraps the index action and reports collected symbols to the execution
// context at the end of each translation unit.
class WrappedIndexAction : public WrapperFrontendAction {
public:
- WrappedIndexAction(std::shared_ptr C,
+ WrappedIndexAction(SymbolMerger &Merger,
+ std::shared_ptr C,
std::unique_ptr Includes,
- const index::IndexingOptions &Opts,
- tooling::ExecutionContext *Ctx)
+ const index::IndexingOptions &Opts)
: WrapperFrontendAction(
index::createIndexingAction(C, Opts, nullptr)),
-Ctx(Ctx), Collector(C), Includes(std::move(Includes)),
+Merger(Merger), Collector(C), Includes(std::move(Includes)),
PragmaHandler(collectIWYUHeaderMaps(this->Includes.get())) {}
std::unique_ptr
@@ -76,17 +102,11 @@
void EndSourceFileAction() override {
WrapperFrontendAction::EndSourceFileAction();
-auto Symbols = Collector->takeSymbols();
-for (const auto &Sym : Symbols) {
- std::string IDStr;
- llvm::raw_string_ostream OS(IDStr);
- OS << Sym.ID;
- Ctx->reportResult(OS.str(), SymbolToYAML(Sym));
-}
+Merger.mergeSymbols(Collector->takeSymbols());
}
private:
- tooling::ExecutionContext *Ctx;
+ SymbolMerger &Merger;
std::shared_ptr Collector;
std::unique_ptr Includes;
std::unique_ptr PragmaHandler;
@@ -104,32 +124,13 @@
addSystemHeadersMapping(Includes.get());
CollectorOpts.Includes = Includes.get();
return new WrappedIndexAction(
-std::make_shared(std::move(CollectorOpts)),
-std::move(Includes), IndexOpts, Ctx);
+Merger, std::make_shared(std::move(CollectorOpts)),
+std::move(Includes), IndexOpts);
}
- tooling::ExecutionContext *Ctx;
+ SymbolMerger &Merger;
};
-// Combine occurrences of the same symbol across translation units.
-SymbolSlab mergeSymbols(tooling::ToolResults *Results) {
- SymbolSlab::Builder UniqueSymbols;
- llvm::BumpPtrAllocator Arena;
- Symbol::Details Scratch;
- Results->forEachResult([&](llvm::StringRef Key, llvm::StringRef Value) {
-Arena.Reset();
-llvm::yaml::Input Yin(Value, &Arena);
-auto Sym = clang::clangd::SymbolFromYAML(Yin, Arena);
-clang::clangd::SymbolID ID;
-Key >> ID;
-if (const auto *Existing = UniqueSymbols.find(ID))
- UniqueSymbols.insert(mergeSymbol(*Existing, Sym, &Scratch));
-else
- UniqueSymbols.insert(Sym);
- });
- return std::move(UniqueSymbols).build();
-}
-
} // namespace
} // namespace clangd
} // namespace clang
@@ -155,18 +156,14 @@
return 1;
}
- // Map phase: emit symbols found in each translation unit.
+ clang::clangd::SymbolMerger Merger;
+ // Emit and merge symbols found in each translation unit.
auto Err = Executor->get()->execute(
- llvm::make_unique(
- Exec
[PATCH] D45478: [clangd] Merge symbols in global-sym-builder on the go
hokein added a comment. Thanks for digging it out! In upstream, we use `InMemoryToolResults` which saves all duplicated "std::string"s into the memory, I think we could optimize `InMemoryToolResults` by using Arena to keep the memory low, I will try it to see whether it can reduce the memory. A bonus point is that we don't need to change any client code. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45478 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45479: [Tooling] Optmized memory usage in InMemoryToolResults.
hokein created this revision.
Herald added subscribers: cfe-commits, klimek.
Repository:
rC Clang
https://reviews.llvm.org/D45479
Files:
include/clang/Tooling/Execution.h
lib/Tooling/Execution.cpp
Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,12 +21,21 @@
llvm::cl::init("standalone"));
void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
- KVResults.push_back({Key.str(), Value.str()});
+ auto Intern = [&](StringRef &V) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+ *R.first = StringsPool.save(V);
+}
+V = *R.first;
+ };
+ Intern(Key);
+ Intern(Value);
+ KVResults.push_back({Key, Value});
}
std::vector>
InMemoryToolResults::AllKVResults() {
- return KVResults;
+ return {};
}
void InMemoryToolResults::forEachResult(
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
namespace clang {
namespace tooling {
@@ -52,13 +53,17 @@
class InMemoryToolResults : public ToolResults {
public:
+ InMemoryToolResults() : StringsPool(Arena) {}
void addResult(StringRef Key, StringRef Value) override;
std::vector> AllKVResults() override;
void forEachResult(llvm::function_ref
Callback) override;
private:
- std::vector> KVResults;
+ llvm::BumpPtrAllocator Arena;
+ llvm::StringSaver StringsPool;
+ llvm::DenseSet Strings;
+ std::vector> KVResults;
};
/// \brief The context of an execution, including the information about
Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,12 +21,21 @@
llvm::cl::init("standalone"));
void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
- KVResults.push_back({Key.str(), Value.str()});
+ auto Intern = [&](StringRef &V) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+ *R.first = StringsPool.save(V);
+}
+V = *R.first;
+ };
+ Intern(Key);
+ Intern(Value);
+ KVResults.push_back({Key, Value});
}
std::vector>
InMemoryToolResults::AllKVResults() {
- return KVResults;
+ return {};
}
void InMemoryToolResults::forEachResult(
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
namespace clang {
namespace tooling {
@@ -52,13 +53,17 @@
class InMemoryToolResults : public ToolResults {
public:
+ InMemoryToolResults() : StringsPool(Arena) {}
void addResult(StringRef Key, StringRef Value) override;
std::vector> AllKVResults() override;
void forEachResult(llvm::function_ref
Callback) override;
private:
- std::vector> KVResults;
+ llvm::BumpPtrAllocator Arena;
+ llvm::StringSaver StringsPool;
+ llvm::DenseSet Strings;
+ std::vector> KVResults;
};
/// \brief The context of an execution, including the information about
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44878: libFuzzer OpenBSD support
dberris added a comment. In the interest of better documentation, can you please update the description of this change to reflect what we're trying to achieve here? Something more descriptive than "libFuzzer OpenBSD Support" that substantiates the change, along with related changes to other projects (sanitizers in particular), would be really nice. https://reviews.llvm.org/D44878 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r329684 - -ftime-report switch support in Clang.
Author: avt77
Date: Tue Apr 10 03:34:13 2018
New Revision: 329684
URL: http://llvm.org/viewvc/llvm-project?rev=329684&view=rev
Log:
-ftime-report switch support in Clang.
The current support of the feature produces only 2 lines in report:
-Some general Code Generation Time;
-Total time of Backend Consumer actions.
This patch extends Clang time report with new lines related to Preprocessor,
Include Filea Search, Parsing, etc.
Differential Revision: https://reviews.llvm.org/D43578
Added:
cfe/trunk/test/Frontend/ftime-report-template-decl.cpp
Modified:
cfe/trunk/include/clang/Frontend/FrontendAction.h
cfe/trunk/include/clang/Lex/HeaderSearch.h
cfe/trunk/include/clang/Parse/Parser.h
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/CodeGen/CodeGenAction.cpp
cfe/trunk/lib/Frontend/ASTMerge.cpp
cfe/trunk/lib/Frontend/CompilerInstance.cpp
cfe/trunk/lib/Frontend/FrontendAction.cpp
cfe/trunk/lib/Frontend/FrontendActions.cpp
cfe/trunk/lib/Lex/HeaderSearch.cpp
cfe/trunk/lib/Lex/PPMacroExpansion.cpp
cfe/trunk/lib/Lex/Pragma.cpp
cfe/trunk/lib/Parse/ParseTemplate.cpp
cfe/trunk/lib/Parse/Parser.cpp
cfe/trunk/lib/Sema/Sema.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/lib/Sema/SemaTemplate.cpp
Modified: cfe/trunk/include/clang/Frontend/FrontendAction.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/FrontendAction.h?rev=329684&r1=329683&r2=329684&view=diff
==
--- cfe/trunk/include/clang/Frontend/FrontendAction.h (original)
+++ cfe/trunk/include/clang/Frontend/FrontendAction.h Tue Apr 10 03:34:13 2018
@@ -45,6 +45,9 @@ private:
StringRef InFile);
protected:
+ static constexpr const char *GroupName = "factions";
+ static constexpr const char *GroupDescription =
+ "= Frontend Actions =";
/// @name Implementation Action Interface
/// @{
Modified: cfe/trunk/include/clang/Lex/HeaderSearch.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/HeaderSearch.h?rev=329684&r1=329683&r2=329684&view=diff
==
--- cfe/trunk/include/clang/Lex/HeaderSearch.h (original)
+++ cfe/trunk/include/clang/Lex/HeaderSearch.h Tue Apr 10 03:34:13 2018
@@ -21,9 +21,10 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/StringMap.h"
-#include "llvm/ADT/StringSet.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Allocator.h"
+#include "llvm/Support/Timer.h"
#include
#include
#include
@@ -31,6 +32,9 @@
#include
#include
+static const char *const IncGroupName = "includefiles";
+static const char *const IncGroupDescription = "= Include Files =";
+
namespace clang {
class DiagnosticsEngine;
Modified: cfe/trunk/include/clang/Parse/Parser.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=329684&r1=329683&r2=329684&view=diff
==
--- cfe/trunk/include/clang/Parse/Parser.h (original)
+++ cfe/trunk/include/clang/Parse/Parser.h Tue Apr 10 03:34:13 2018
@@ -2851,4 +2851,6 @@ private:
} // end namespace clang
+static const char *const GroupName = "clangparser";
+static const char *const GroupDescription = "= Clang Parser =";
#endif
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=329684&r1=329683&r2=329684&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Tue Apr 10 03:34:13 2018
@@ -304,6 +304,9 @@ class Sema {
bool shouldLinkPossiblyHiddenDecl(LookupResult &Old, const NamedDecl *New);
public:
+ static constexpr const char *GroupName = "sema";
+ static constexpr const char *GroupDescription = "= Sema =";
+
typedef OpaquePtr DeclGroupPtrTy;
typedef OpaquePtr TemplateTy;
typedef OpaquePtr TypeTy;
Modified: cfe/trunk/lib/CodeGen/CodeGenAction.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenAction.cpp?rev=329684&r1=329683&r2=329684&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenAction.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenAction.cpp Tue Apr 10 03:34:13 2018
@@ -23,6 +23,7 @@
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/FrontendDiagnostic.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Rewrite/Frontend/FrontendActions.h"
#include "llvm/Bitcode/BitcodeReader.h"
#include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
#include "llvm/IR/Debu
r329685 - [Tooling] fix UB when interpolating compile commands with an empty index
Author: sammccall
Date: Tue Apr 10 03:36:46 2018
New Revision: 329685
URL: http://llvm.org/viewvc/llvm-project?rev=329685&view=rev
Log:
[Tooling] fix UB when interpolating compile commands with an empty index
Modified:
cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp
Modified: cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp?rev=329685&r1=329684&r2=329685&view=diff
==
--- cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp (original)
+++ cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp Tue Apr 10
03:36:46 2018
@@ -373,8 +373,8 @@ private:
ArrayRef
indexLookup(StringRef Key, const std::vector &Idx) const {
// Use pointers as iteratiors to ease conversion of result to ArrayRef.
-auto Range =
-std::equal_range(&Idx[0], &Idx[Idx.size()], Key, Less());
+auto Range = std::equal_range(Idx.data(), Idx.data() + Idx.size(), Key,
+ Less());
return {Range.first, Range.second};
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r329580 - [Tooling] A CompilationDatabase wrapper that infers header commands.
Apologies, missed this. r329685 should fix. On Tue, Apr 10, 2018 at 12:53 AM Galina Kistanova wrote: > Hello Sam, > > It looks like this commit added broken tests to one of our builders: > > http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/8957/steps/test-check-all/logs/stdio > > Failing Tests (5): > Clang-Unit :: Tooling/./ToolingTests.exe/InterpolateTest.Case > Clang-Unit :: Tooling/./ToolingTests.exe/InterpolateTest.Language > Clang-Unit :: Tooling/./ToolingTests.exe/InterpolateTest.Nearby > Clang-Unit :: Tooling/./ToolingTests.exe/InterpolateTest.Strip > . . . > Please have a look? > > The builder was red and did not send notifications. > > Thanks > > Galina > > On Mon, Apr 9, 2018 at 8:17 AM, Sam McCall via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: sammccall >> Date: Mon Apr 9 08:17:39 2018 >> New Revision: 329580 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=329580&view=rev >> Log: >> [Tooling] A CompilationDatabase wrapper that infers header commands. >> >> Summary: >> The wrapper finds the closest matching compile command using filename >> heuristics >> and makes minimal tweaks so it can be used with the header. >> >> Subscribers: klimek, mgorny, cfe-commits >> >> Differential Revision: https://reviews.llvm.org/D45006 >> >> Added: >> cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp >> Modified: >> cfe/trunk/include/clang/Tooling/CompilationDatabase.h >> cfe/trunk/lib/Tooling/CMakeLists.txt >> cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp >> >> Modified: cfe/trunk/include/clang/Tooling/CompilationDatabase.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/CompilationDatabase.h?rev=329580&r1=329579&r2=329580&view=diff >> >> == >> --- cfe/trunk/include/clang/Tooling/CompilationDatabase.h (original) >> +++ cfe/trunk/include/clang/Tooling/CompilationDatabase.h Mon Apr 9 >> 08:17:39 2018 >> @@ -213,6 +213,13 @@ private: >>std::vector CompileCommands; >> }; >> >> +/// Returns a wrapped CompilationDatabase that defers to the provided >> one, >> +/// but getCompileCommands() will infer commands for unknown files. >> +/// The return value of getAllFiles() or getAllCompileCommands() is >> unchanged. >> +/// See InterpolatingCompilationDatabase.cpp for details on heuristics. >> +std::unique_ptr >> +inferMissingCompileCommands(std::unique_ptr); >> + >> } // namespace tooling >> } // namespace clang >> >> >> Modified: cfe/trunk/lib/Tooling/CMakeLists.txt >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/CMakeLists.txt?rev=329580&r1=329579&r2=329580&view=diff >> >> == >> --- cfe/trunk/lib/Tooling/CMakeLists.txt (original) >> +++ cfe/trunk/lib/Tooling/CMakeLists.txt Mon Apr 9 08:17:39 2018 >> @@ -15,6 +15,7 @@ add_clang_library(clangTooling >>Execution.cpp >>FileMatchTrie.cpp >>FixIt.cpp >> + InterpolatingCompilationDatabase.cpp >>JSONCompilationDatabase.cpp >>Refactoring.cpp >>RefactoringCallbacks.cpp >> >> Added: cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp?rev=329580&view=auto >> >> == >> --- cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp (added) >> +++ cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp Mon Apr 9 >> 08:17:39 2018 >> @@ -0,0 +1,458 @@ >> +//===- InterpolatingCompilationDatabase.cpp -*- C++ >> -*-===// >> +// >> +// The LLVM Compiler Infrastructure >> +// >> +// This file is distributed under the University of Illinois Open Source >> +// License. See LICENSE.TXT for details. >> +// >> >> +//===--===// >> +// >> +// InterpolatingCompilationDatabase wraps another CompilationDatabase and >> +// attempts to heuristically determine appropriate compile commands for >> files >> +// that are not included, such as headers or newly created files. >> +// >> +// Motivating cases include: >> +// Header files that live next to their implementation files. These >> typically >> +// share a base filename. (libclang/CXString.h, libclang/CXString.cpp). >> +// Some projects separate headers from includes. Filenames still >> typically >> +// match, maybe other path segments too. (include/llvm/IR/Use.h, >> lib/IR/Use.cc). >> +// Matches are sometimes only approximate (Sema.h, SemaDecl.cpp). This >> goes >> +// for directories too (Support/Unix/Process.inc, >> lib/Support/Process.cpp). >> +// Even if we can't find a "right" compile command, even a random one >> from >> +// the project will tend to get important flags like -I and -x right. >> +//
[PATCH] D45482: [clangd] Match AST and Index label for template Symbols
ilya-biryukov created this revision.
ilya-biryukov added reviewers: sammccall, hokein.
Herald added subscribers: MaskRay, ioeric, jkorous-apple, klimek.
Previsouly, class completions items from the index were missing
template parameters in both the snippet and the label.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45482
Files:
clangd/index/SymbolCollector.cpp
unittests/clangd/FileIndexTests.cpp
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -195,6 +195,42 @@
EXPECT_TRUE(SeenSymbol);
}
+TEST(FileIndexTest, TemplateParamsInLabel) {
+ auto Source = R"cpp(
+template
+class vector {
+};
+
+template
+vector make_vector(Ty* begin, Ty* end) {}
+)cpp";
+
+ FileIndex M;
+ M.update("f", build("f", Source).getPointer());
+
+ FuzzyFindRequest Req;
+ Req.Query = "";
+ bool SeenVector = false;
+ bool SeenMakeVector = false;
+ M.fuzzyFind(Req, [&](const Symbol &Sym) {
+if (Sym.Name == "vector") {
+ EXPECT_EQ(Sym.CompletionLabel, "vector");
+ EXPECT_EQ(Sym.CompletionSnippetInsertText, "vector<${1:class Ty}>");
+ SeenVector = true;
+ return;
+}
+
+if (Sym.Name == "make_vector") {
+ EXPECT_EQ(Sym.CompletionLabel, "make_vector(Ty *begin, Ty *end)");
+ EXPECT_EQ(Sym.CompletionSnippetInsertText,
+"make_vector(${1:Ty *begin}, ${2:Ty *end})");
+ SeenMakeVector = true;
+}
+ });
+ EXPECT_TRUE(SeenVector);
+ EXPECT_TRUE(SeenMakeVector);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -14,6 +14,7 @@
#include "../URI.h"
#include "CanonicalIncludes.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Index/IndexSymbol.h"
@@ -26,6 +27,22 @@
namespace clangd {
namespace {
+/// If \p ND is a template specialization, returns the primary template.
+/// Otherwise, returns \p ND.
+const NamedDecl &getTemplateOrThis(const NamedDecl &ND) {
+ if (auto Cls = dyn_cast(&ND)) {
+if (auto T = Cls->getDescribedTemplate())
+ return *T;
+return ND;
+ }
+ if (auto Func = dyn_cast(&ND)) {
+if (auto T = Func->getPrimaryTemplate())
+ return *T;
+return ND;
+ }
+ return ND;
+}
+
// Returns a URI of \p Path. Firstly, this makes the \p Path absolute using the
// current working directory of the given SourceManager if the Path is not an
// absolute path. If failed, this resolves relative paths against \p
FallbackDir
@@ -310,7 +327,9 @@
// Add completion info.
// FIXME: we may want to choose a different redecl, or combine from several.
assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
- CodeCompletionResult SymbolCompletion(&ND, 0);
+ // We call getTemplateOrThis, since this is what clang's code completion gets
+ // from the lookup in an actual run.
+ CodeCompletionResult SymbolCompletion(&getTemplateOrThis(ND), 0);
const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
*ASTCtx, *PP, CodeCompletionContext::CCC_Name, *CompletionAllocator,
*CompletionTUInfo,
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -195,6 +195,42 @@
EXPECT_TRUE(SeenSymbol);
}
+TEST(FileIndexTest, TemplateParamsInLabel) {
+ auto Source = R"cpp(
+template
+class vector {
+};
+
+template
+vector make_vector(Ty* begin, Ty* end) {}
+)cpp";
+
+ FileIndex M;
+ M.update("f", build("f", Source).getPointer());
+
+ FuzzyFindRequest Req;
+ Req.Query = "";
+ bool SeenVector = false;
+ bool SeenMakeVector = false;
+ M.fuzzyFind(Req, [&](const Symbol &Sym) {
+if (Sym.Name == "vector") {
+ EXPECT_EQ(Sym.CompletionLabel, "vector");
+ EXPECT_EQ(Sym.CompletionSnippetInsertText, "vector<${1:class Ty}>");
+ SeenVector = true;
+ return;
+}
+
+if (Sym.Name == "make_vector") {
+ EXPECT_EQ(Sym.CompletionLabel, "make_vector(Ty *begin, Ty *end)");
+ EXPECT_EQ(Sym.CompletionSnippetInsertText,
+"make_vector(${1:Ty *begin}, ${2:Ty *end})");
+ SeenMakeVector = true;
+}
+ });
+ EXPECT_TRUE(SeenVector);
+ EXPECT_TRUE(SeenMakeVector);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -14,6 +14,7 @@
#include "../URI.h"
#include "CanonicalIncludes.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
[PATCH] D45479: [Tooling] Optmized memory usage in InMemoryToolResults.
hokein updated this revision to Diff 141822.
hokein added a comment.
Update.
Repository:
rC Clang
https://reviews.llvm.org/D45479
Files:
include/clang/Tooling/Execution.h
lib/Tooling/AllTUsExecution.cpp
lib/Tooling/Execution.cpp
Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,10 +21,19 @@
llvm::cl::init("standalone"));
void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
- KVResults.push_back({Key.str(), Value.str()});
+ auto Intern = [&](StringRef &V) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+ *R.first = StringsPool.save(V);
+}
+V = *R.first;
+ };
+ Intern(Key);
+ Intern(Value);
+ KVResults.push_back({Key, Value});
}
-std::vector>
+std::vector>
InMemoryToolResults::AllKVResults() {
return KVResults;
}
Index: lib/Tooling/AllTUsExecution.cpp
===
--- lib/Tooling/AllTUsExecution.cpp
+++ lib/Tooling/AllTUsExecution.cpp
@@ -36,7 +36,8 @@
Results.addResult(Key, Value);
}
- std::vector> AllKVResults() override {
+ std::vector>
+ AllKVResults() override {
return Results.AllKVResults();
}
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
namespace clang {
namespace tooling {
@@ -45,20 +46,27 @@
public:
virtual ~ToolResults() = default;
virtual void addResult(StringRef Key, StringRef Value) = 0;
- virtual std::vector> AllKVResults() = 0;
+ virtual std::vector>
+ AllKVResults() = 0;
virtual void forEachResult(
llvm::function_ref Callback) = 0;
};
class InMemoryToolResults : public ToolResults {
public:
+ InMemoryToolResults() : StringsPool(Arena) {}
void addResult(StringRef Key, StringRef Value) override;
- std::vector> AllKVResults() override;
+ std::vector>
+ AllKVResults() override;
void forEachResult(llvm::function_ref
Callback) override;
private:
- std::vector> KVResults;
+ llvm::BumpPtrAllocator Arena;
+ llvm::StringSaver StringsPool;
+ llvm::DenseSet Strings;
+
+ std::vector> KVResults;
};
/// \brief The context of an execution, including the information about
Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,10 +21,19 @@
llvm::cl::init("standalone"));
void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
- KVResults.push_back({Key.str(), Value.str()});
+ auto Intern = [&](StringRef &V) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+ *R.first = StringsPool.save(V);
+}
+V = *R.first;
+ };
+ Intern(Key);
+ Intern(Value);
+ KVResults.push_back({Key, Value});
}
-std::vector>
+std::vector>
InMemoryToolResults::AllKVResults() {
return KVResults;
}
Index: lib/Tooling/AllTUsExecution.cpp
===
--- lib/Tooling/AllTUsExecution.cpp
+++ lib/Tooling/AllTUsExecution.cpp
@@ -36,7 +36,8 @@
Results.addResult(Key, Value);
}
- std::vector> AllKVResults() override {
+ std::vector>
+ AllKVResults() override {
return Results.AllKVResults();
}
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
namespace clang {
namespace tooling {
@@ -45,20 +46,27 @@
public:
virtual ~ToolResults() = default;
virtual void addResult(StringRef Key, StringRef Value) = 0;
- virtual std::vector> AllKVResults() = 0;
+ virtual std::vector>
+ AllKVResults() = 0;
virtual void forEachResult(
llvm::function_ref Callback) = 0;
};
class InMemoryToolResults : public ToolResults {
public:
+ InMemoryToolResults() : StringsPool(Arena) {}
void addResult(StringRef Key, StringRef Value) override;
- std::vector> AllKVResults() override;
+ std::vector>
+ AllKVResults() override;
void forEachResult(llvm::function_ref
Callback) override;
private:
- std::vector> KVResults;
+ llvm::BumpPtrAllocator Arena;
+ llvm::StringSaver StringsPool;
+ llvm::DenseSet Strings;
+
+ std::vector> KVResults;
};
/// \brief The context of an execution, including the information about
___
[PATCH] D43815: CodeGen tests - typo fixes
GBuella added a comment. See: https://reviews.llvm.org/rL329689 Repository: rC Clang https://reviews.llvm.org/D43815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43815: CodeGen tests - typo fixes
GBuella closed this revision. GBuella added a comment. Commited r329688 https://reviews.llvm.org/rL329689 Repository: rC Clang https://reviews.llvm.org/D43815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43764: [clang-apply-replacements] Convert tooling::Replacements to tooling::AtomicChange for conflict resolving of changes, code cleanup, and code formatting.
malcolm.parsons added a comment. In https://reviews.llvm.org/D43764#1062748, @jdemeule wrote: > Ok, I understand the problem. > Previously, during the deduplication step, replacements per file where > sorted and it is not the case anymore. > That means now, clang-apply-replacement is highly dependant of reading file > order during error reporting. > I mainly see 2 options (I can miss others), rewrite the test (e.g merging > yaml file together) or sort the replacements per file to fix the order of > application. > > Can I ask you what solution you prefer? I'd prefer sorting, so that results will be consistent when the tool is used on real data. https://reviews.llvm.org/D43764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45442: Parse .h files as objective-c++ if we don't have a compile command.
ilya-biryukov added a comment.
ObjC++ definitely seems like a nicer default. Unfortunately, we'll start
getting ObjC++ completion results, which may be confusing. But that seems like
a smaller evil.
Is there an easy way to add a regression test that checks we don't get any
errors for headers without compile command with C++/ObjC++ code?
Comment at: clangd/GlobalCompilationDatabase.cpp:24
+ // Parsing as Objective C++ is friendly to more cases.
+ if (llvm::sys::path::extension(File) == ".h")
+Argv.push_back("-xobjective-c++-header");
Maybe use `.equals_lower(".h")` instead? Just in case.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45442
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45483: [NEON] Support vfma_n and vfms_n intrinsics
kosarev created this revision.
kosarev added reviewers: az, sbaranga, t.p.northover.
kosarev added a project: clang.
Herald added subscribers: javed.absar, rengolin.
https://reviews.llvm.org/D45483
Files:
include/clang/Basic/arm_neon.td
test/CodeGen/aarch64-neon-2velem.c
Index: test/CodeGen/aarch64-neon-2velem.c
===
--- test/CodeGen/aarch64-neon-2velem.c
+++ test/CodeGen/aarch64-neon-2velem.c
@@ -3083,6 +3083,17 @@
return vfma_n_f32(a, b, n);
}
+// CHECK-LABEL: @test_vfma_n_f64(
+// CHECK: [[VECINIT_I:%.*]] = insertelement <1 x double> undef, double %n,
i32 0
+// CHECK: [[TMP0:%.*]] = bitcast <1 x double> %a to <8 x i8>
+// CHECK: [[TMP1:%.*]] = bitcast <1 x double> %b to <8 x i8>
+// CHECK: [[TMP2:%.*]] = bitcast <1 x double> [[VECINIT_I]] to <8 x i8>
+// CHECK: [[TMP3:%.*]] = call <1 x double> @llvm.fma.v1f64(<1 x double> %b,
<1 x double> [[VECINIT_I]], <1 x double> %a)
+// CHECK: ret <1 x double> [[TMP3]]
+float64x1_t test_vfma_n_f64(float64x1_t a, float64x1_t b, float64_t n) {
+ return vfma_n_f64(a, b, n);
+}
+
// CHECK-LABEL: @test_vfmaq_n_f32(
// CHECK: [[VECINIT_I:%.*]] = insertelement <4 x float> undef, float %n, i32 0
// CHECK: [[VECINIT1_I:%.*]] = insertelement <4 x float> [[VECINIT_I]],
float %n, i32 1
@@ -3110,6 +3121,18 @@
return vfms_n_f32(a, b, n);
}
+// CHECK-LABEL: @test_vfms_n_f64(
+// CHECK: [[SUB_I:%.*]] = fsub <1 x double> , %b
+// CHECK: [[VECINIT_I:%.*]] = insertelement <1 x double> undef, double %n,
i32 0
+// CHECK: [[TMP0:%.*]] = bitcast <1 x double> %a to <8 x i8>
+// CHECK: [[TMP1:%.*]] = bitcast <1 x double> [[SUB_I]] to <8 x i8>
+// CHECK: [[TMP2:%.*]] = bitcast <1 x double> [[VECINIT_I]] to <8 x i8>
+// CHECK: [[TMP3:%.*]] = call <1 x double> @llvm.fma.v1f64(<1 x double>
[[SUB_I]], <1 x double> [[VECINIT_I]], <1 x double> %a)
+// CHECK: ret <1 x double> [[TMP3]]
+float64x1_t test_vfms_n_f64(float64x1_t a, float64x1_t b, float64_t n) {
+ return vfms_n_f64(a, b, n);
+}
+
// CHECK-LABEL: @test_vfmsq_n_f32(
// CHECK: [[SUB_I:%.*]] = fsub <4 x float> , %b
// CHECK: [[VECINIT_I:%.*]] = insertelement <4 x float> undef, float %n, i32 0
Index: include/clang/Basic/arm_neon.td
===
--- include/clang/Basic/arm_neon.td
+++ include/clang/Basic/arm_neon.td
@@ -621,8 +621,8 @@
// MUL, MLA, MLS, FMA, FMS definitions with scalar argument
def VMUL_N_A64 : IOpInst<"vmul_n", "dds", "Qd", OP_MUL_N>;
-def FMLA_N : SOpInst<"vfma_n", "ddds", "fQfQd", OP_FMLA_N>;
-def FMLS_N : SOpInst<"vfms_n", "ddds", "fQfQd", OP_FMLS_N>;
+def FMLA_N : SOpInst<"vfma_n", "ddds", "fdQfQd", OP_FMLA_N>;
+def FMLS_N : SOpInst<"vfms_n", "ddds", "fdQfQd", OP_FMLS_N>;
def MLA_N : SOpInst<"vmla_n", "ddds", "Qd", OP_MLA_N>;
def MLS_N : SOpInst<"vmls_n", "ddds", "Qd", OP_MLS_N>;
Index: test/CodeGen/aarch64-neon-2velem.c
===
--- test/CodeGen/aarch64-neon-2velem.c
+++ test/CodeGen/aarch64-neon-2velem.c
@@ -3083,6 +3083,17 @@
return vfma_n_f32(a, b, n);
}
+// CHECK-LABEL: @test_vfma_n_f64(
+// CHECK: [[VECINIT_I:%.*]] = insertelement <1 x double> undef, double %n, i32 0
+// CHECK: [[TMP0:%.*]] = bitcast <1 x double> %a to <8 x i8>
+// CHECK: [[TMP1:%.*]] = bitcast <1 x double> %b to <8 x i8>
+// CHECK: [[TMP2:%.*]] = bitcast <1 x double> [[VECINIT_I]] to <8 x i8>
+// CHECK: [[TMP3:%.*]] = call <1 x double> @llvm.fma.v1f64(<1 x double> %b, <1 x double> [[VECINIT_I]], <1 x double> %a)
+// CHECK: ret <1 x double> [[TMP3]]
+float64x1_t test_vfma_n_f64(float64x1_t a, float64x1_t b, float64_t n) {
+ return vfma_n_f64(a, b, n);
+}
+
// CHECK-LABEL: @test_vfmaq_n_f32(
// CHECK: [[VECINIT_I:%.*]] = insertelement <4 x float> undef, float %n, i32 0
// CHECK: [[VECINIT1_I:%.*]] = insertelement <4 x float> [[VECINIT_I]], float %n, i32 1
@@ -3110,6 +3121,18 @@
return vfms_n_f32(a, b, n);
}
+// CHECK-LABEL: @test_vfms_n_f64(
+// CHECK: [[SUB_I:%.*]] = fsub <1 x double> , %b
+// CHECK: [[VECINIT_I:%.*]] = insertelement <1 x double> undef, double %n, i32 0
+// CHECK: [[TMP0:%.*]] = bitcast <1 x double> %a to <8 x i8>
+// CHECK: [[TMP1:%.*]] = bitcast <1 x double> [[SUB_I]] to <8 x i8>
+// CHECK: [[TMP2:%.*]] = bitcast <1 x double> [[VECINIT_I]] to <8 x i8>
+// CHECK: [[TMP3:%.*]] = call <1 x double> @llvm.fma.v1f64(<1 x double> [[SUB_I]], <1 x double> [[VECINIT_I]], <1 x double> %a)
+// CHECK: ret <1 x double> [[TMP3]]
+float64x1_t test_vfms_n_f64(float64x1_t a, float64x1_t b, float64_t n) {
+ return vfms_n_f64(a, b, n);
+}
+
// CHECK-LABEL: @test_vfmsq_n_f32(
// CHECK: [[SUB_I:%.*]] = fsub <4 x float> , %b
// CHECK: [[VECINIT_I:%.*]] = insertelement <4 x float> undef, float %n, i32 0
Index: include/clang/Basic/arm_neon.td
===
--- include/clang/Basic/arm_neon.td
+++ inc
[PATCH] D45416: [analyzer] ExprEngine: model GCC inline asm rvalue cast outputs
a.sidorin added a comment. > The ultimate solution would probably be to add a fake cloned asm statement to > the CFG (instead of the real asm statement) that would point to the correct > output child-expression(s) that are untouched themselves but simply have > their noop casts removed. I have some concerns about this solution. It will result in difference between AST nodes and nodes that user will receive during analysis and I'm not sure that it is good. What is even worse here is that a lot of AST stuff won't work properly - ParentMap, for example. > Or we could try Looks like something is missed here :) Repository: rC Clang https://reviews.llvm.org/D45416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r329693 - The test was fixed.
Author: avt77
Date: Tue Apr 10 05:17:01 2018
New Revision: 329693
URL: http://llvm.org/viewvc/llvm-project?rev=329693&view=rev
Log:
The test was fixed.
Modified:
cfe/trunk/test/Frontend/ftime-report-template-decl.cpp
Modified: cfe/trunk/test/Frontend/ftime-report-template-decl.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/ftime-report-template-decl.cpp?rev=329693&r1=329692&r2=329693&view=diff
==
--- cfe/trunk/test/Frontend/ftime-report-template-decl.cpp (original)
+++ cfe/trunk/test/Frontend/ftime-report-template-decl.cpp Tue Apr 10 05:17:01
2018
@@ -137,7 +137,7 @@ void Instantiate() {
}
// CHECK: = Clang Parser =
-// CHECK: ---User Time--- --User+System-- ---Wall Time--- --- Name ---
+// CHECK: ---User Time---
// CHECK: Parse Top Level Decl
// CHECK: Parse Template
// CHECK: Parse Function Definition
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros
markoshorro added a comment. This feature should have been merged time ago, any news? Repository: rL LLVM https://reviews.llvm.org/D28462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45485: [CMake] Unbreak BUILD_SHARED_LIBS workflow
davezarzycki created this revision. davezarzycki added reviewers: mspertus, beanz, gottesmm. Herald added a subscriber: mgorny. Due to recent timer changes, we need to add LLVM's `Core` library to avoid undefined symbol errors at link time (`llvm::TimePassesIsEnabled`) when using LLVM's BUILD_SHARED_LIBS workflow. Repository: rC Clang https://reviews.llvm.org/D45485 Files: lib/Frontend/CMakeLists.txt lib/Lex/CMakeLists.txt lib/Parse/CMakeLists.txt lib/Sema/CMakeLists.txt Index: lib/Sema/CMakeLists.txt === --- lib/Sema/CMakeLists.txt +++ lib/Sema/CMakeLists.txt @@ -1,4 +1,5 @@ set(LLVM_LINK_COMPONENTS + Core Support ) Index: lib/Parse/CMakeLists.txt === --- lib/Parse/CMakeLists.txt +++ lib/Parse/CMakeLists.txt @@ -1,4 +1,5 @@ set(LLVM_LINK_COMPONENTS + Core MC MCParser Support Index: lib/Lex/CMakeLists.txt === --- lib/Lex/CMakeLists.txt +++ lib/Lex/CMakeLists.txt @@ -1,6 +1,9 @@ # TODO: Add -maltivec when ARCH is PowerPC. -set(LLVM_LINK_COMPONENTS support) +set(LLVM_LINK_COMPONENTS + Core + support + ) add_clang_library(clangLex HeaderMap.cpp Index: lib/Frontend/CMakeLists.txt === --- lib/Frontend/CMakeLists.txt +++ lib/Frontend/CMakeLists.txt @@ -1,6 +1,7 @@ add_subdirectory(Rewrite) set(LLVM_LINK_COMPONENTS + Core BitReader Option ProfileData Index: lib/Sema/CMakeLists.txt === --- lib/Sema/CMakeLists.txt +++ lib/Sema/CMakeLists.txt @@ -1,4 +1,5 @@ set(LLVM_LINK_COMPONENTS + Core Support ) Index: lib/Parse/CMakeLists.txt === --- lib/Parse/CMakeLists.txt +++ lib/Parse/CMakeLists.txt @@ -1,4 +1,5 @@ set(LLVM_LINK_COMPONENTS + Core MC MCParser Support Index: lib/Lex/CMakeLists.txt === --- lib/Lex/CMakeLists.txt +++ lib/Lex/CMakeLists.txt @@ -1,6 +1,9 @@ # TODO: Add -maltivec when ARCH is PowerPC. -set(LLVM_LINK_COMPONENTS support) +set(LLVM_LINK_COMPONENTS + Core + support + ) add_clang_library(clangLex HeaderMap.cpp Index: lib/Frontend/CMakeLists.txt === --- lib/Frontend/CMakeLists.txt +++ lib/Frontend/CMakeLists.txt @@ -1,6 +1,7 @@ add_subdirectory(Rewrite) set(LLVM_LINK_COMPONENTS + Core BitReader Option ProfileData ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45485: [CMake] Unbreak BUILD_SHARED_LIBS workflow
thakis added a comment. Can you link to the recent timer changes? Repository: rC Clang https://reviews.llvm.org/D45485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r329695 - s/LLVM_ON_WIN32/_WIN32/, clang-tools-extra
Author: nico
Date: Tue Apr 10 06:14:03 2018
New Revision: 329695
URL: http://llvm.org/viewvc/llvm-project?rev=329695&view=rev
Log:
s/LLVM_ON_WIN32/_WIN32/, clang-tools-extra
LLVM_ON_WIN32 is set exactly with MSVC and MinGW (but not Cygwin) in
HandleLLVMOptions.cmake, which is where _WIN32 defined too. Just use the
default macro instead of a reinvented one.
See thread "Replacing LLVM_ON_WIN32 with just _WIN32" on llvm-dev and cfe-dev.
No intended behavior change.
Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
clang-tools-extra/trunk/unittests/clangd/URITests.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=329695&r1=329694&r2=329695&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Tue Apr 10 06:14:03 2018
@@ -38,7 +38,7 @@ public:
"Expect URI body to be an absolute path starting with '/': " + Body,
llvm::inconvertibleErrorCode());
Body = Body.ltrim('/');
-#ifdef LLVM_ON_WIN32
+#ifdef _WIN32
constexpr char TestDir[] = "C:\\clangd-test";
#else
constexpr char TestDir[] = "/clangd-test";
Modified: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp?rev=329695&r1=329694&r2=329695&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp Tue Apr
10 06:14:03 2018
@@ -286,7 +286,7 @@ TEST_F(SymbolCollectorTest, SymbolRelati
UnorderedElementsAre(AllOf(QName("Foo"),
DeclURI(TestHeaderURI;
}
-#ifndef LLVM_ON_WIN32
+#ifndef _WIN32
TEST_F(SymbolCollectorTest, CustomURIScheme) {
// Use test URI scheme from URITests.cpp
CollectorOpts.URISchemes.insert(CollectorOpts.URISchemes.begin(),
"unittest");
@@ -571,7 +571,7 @@ TEST_F(SymbolCollectorTest, IncludeHeade
IncludeHeader(TestHeaderURI;
}
-#ifndef LLVM_ON_WIN32
+#ifndef _WIN32
TEST_F(SymbolCollectorTest, CanonicalSTLHeader) {
CollectorOpts.CollectIncludePath = true;
CanonicalIncludes Includes;
Modified: clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TestFS.cpp?rev=329695&r1=329694&r2=329695&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/TestFS.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TestFS.cpp Tue Apr 10 06:14:03 2018
@@ -45,7 +45,7 @@ MockCompilationDatabase::getCompileComma
}
const char *testRoot() {
-#ifdef LLVM_ON_WIN32
+#ifdef _WIN32
return "C:\\clangd-test";
#else
return "/clangd-test";
Modified: clang-tools-extra/trunk/unittests/clangd/URITests.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/URITests.cpp?rev=329695&r1=329694&r2=329695&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/URITests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/URITests.cpp Tue Apr 10 06:14:03
2018
@@ -93,7 +93,7 @@ std::string resolveOrDie(const URI &U, l
}
TEST(URITest, Create) {
-#ifdef LLVM_ON_WIN32
+#ifdef _WIN32
EXPECT_THAT(createOrDie("c:\\x\\y\\z"), "file:///c%3a/x/y/z");
#else
EXPECT_THAT(createOrDie("/x/y/z"), "file:///x/y/z");
@@ -162,7 +162,7 @@ TEST(URITest, ParseFailed) {
}
TEST(URITest, Resolve) {
-#ifdef LLVM_ON_WIN32
+#ifdef _WIN32
EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), "c:\\x\\y\\z");
#else
EXPECT_EQ(resolveOrDie(parseOrDie("file:/a/b/c")), "/a/b/c");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45485: [CMake] Unbreak BUILD_SHARED_LIBS workflow
davezarzycki added a comment. I haven't proven that r329684 is the source of the regression, but it is the only timer related change in the last 24 hours as far as I can tell. Repository: rC Clang https://reviews.llvm.org/D45485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45165: Use llvm::sys::fs::real_path() in clang.
hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D45165 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45442: Parse .h files as objective-c++ if we don't have a compile command.
sammccall added a comment.
Will try to add a test.
Comment at: clangd/GlobalCompilationDatabase.cpp:24
+ // Parsing as Objective C++ is friendly to more cases.
+ if (llvm::sys::path::extension(File) == ".h")
+Argv.push_back("-xobjective-c++-header");
ilya-biryukov wrote:
> Maybe use `.equals_lower(".h")` instead? Just in case.
.H is (unambiguously) c++, and will be treated as such by clang.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45442
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45479: [Tooling] Optimize memory usage in InMemoryToolResults.
ilya-biryukov added a comment. This definitely helps with memory consumption of clangd's global-symbol-indexer, I can now run it over LLVM on MacBook without it swapping out of RAM. The physical memory usage goes up to 3GB at some point, before it was > 20GB. Strictly speaking, interning the strings is probably more work for some workloads, so I'm not sure it's a better in all cases. On average this LG, though, so LGTM from my side. Repository: rC Clang https://reviews.llvm.org/D45479 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45485: [CMake] Unbreak BUILD_SHARED_LIBS workflow
thakis added a comment. Thanks for the pointer! I commented on https://reviews.llvm.org/D43578; I'm hoping there's a fix that doesn't require making so many libs depend on clang's IR library. Repository: rC Clang https://reviews.llvm.org/D45485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45165: Use llvm::sys::fs::real_path() in clang.
thakis added a comment. r329698, thanks! https://reviews.llvm.org/D45165 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r329698 - Use llvm::sys::fs::real_path() in clang.
Author: nico
Date: Tue Apr 10 06:36:38 2018
New Revision: 329698
URL: http://llvm.org/viewvc/llvm-project?rev=329698&view=rev
Log:
Use llvm::sys::fs::real_path() in clang.
No expected behavior change.
https://reviews.llvm.org/D45165
Modified:
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/lib/Frontend/ModuleDependencyCollector.cpp
Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=329698&r1=329697&r2=329698&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Tue Apr 10 06:36:38 2018
@@ -534,23 +534,9 @@ StringRef FileManager::getCanonicalName(
StringRef CanonicalName(Dir->getName());
-#ifdef LLVM_ON_UNIX
- char CanonicalNameBuf[PATH_MAX];
- if (realpath(Dir->getName().str().c_str(), CanonicalNameBuf))
+ SmallString CanonicalNameBuf;
+ if (!llvm::sys::fs::real_path(Dir->getName(), CanonicalNameBuf))
CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
-#else
- SmallString<256> CanonicalNameBuf(CanonicalName);
- llvm::sys::fs::make_absolute(CanonicalNameBuf);
- llvm::sys::path::native(CanonicalNameBuf);
- // We've run into needing to remove '..' here in the wild though, so
- // remove it.
- // On Windows, symlinks are significantly less prevalent, so removing
- // '..' is pretty safe.
- // Ideally we'd have an equivalent of `realpath` and could implement
- // sys::fs::canonical across all the platforms.
- llvm::sys::path::remove_dots(CanonicalNameBuf, /* remove_dot_dot */ true);
- CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
-#endif
CanonicalDirNames.insert(std::make_pair(Dir, CanonicalName));
return CanonicalName;
Modified: cfe/trunk/lib/Frontend/ModuleDependencyCollector.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ModuleDependencyCollector.cpp?rev=329698&r1=329697&r2=329698&view=diff
==
--- cfe/trunk/lib/Frontend/ModuleDependencyCollector.cpp (original)
+++ cfe/trunk/lib/Frontend/ModuleDependencyCollector.cpp Tue Apr 10 06:36:38
2018
@@ -97,24 +97,6 @@ struct ModuleDependencyMMCallbacks : pub
}
-// TODO: move this to Support/Path.h and check for HAVE_REALPATH?
-static bool real_path(StringRef SrcPath, SmallVectorImpl &RealPath) {
-#ifdef LLVM_ON_UNIX
- char CanonicalPath[PATH_MAX];
-
- // TODO: emit a warning in case this fails...?
- if (!realpath(SrcPath.str().c_str(), CanonicalPath))
-return false;
-
- SmallString<256> RPath(CanonicalPath);
- RealPath.swap(RPath);
- return true;
-#else
- // FIXME: Add support for systems without realpath.
- return false;
-#endif
-}
-
void ModuleDependencyCollector::attachToASTReader(ASTReader &R) {
R.addListener(llvm::make_unique(*this));
}
@@ -129,7 +111,7 @@ void ModuleDependencyCollector::attachTo
static bool isCaseSensitivePath(StringRef Path) {
SmallString<256> TmpDest = Path, UpperDest, RealDest;
// Remove component traversals, links, etc.
- if (!real_path(Path, TmpDest))
+ if (llvm::sys::fs::real_path(Path, TmpDest))
return true; // Current default value in vfs.yaml
Path = TmpDest;
@@ -139,7 +121,7 @@ static bool isCaseSensitivePath(StringRe
// already expects when sensitivity isn't setup.
for (auto &C : Path)
UpperDest.push_back(toUppercase(C));
- if (real_path(UpperDest, RealDest) && Path.equals(RealDest))
+ if (!llvm::sys::fs::real_path(UpperDest, RealDest) && Path.equals(RealDest))
return false;
return true;
}
@@ -189,7 +171,7 @@ bool ModuleDependencyCollector::getRealP
// Computing the real path is expensive, cache the search through the
// parent path directory.
if (DirWithSymLink == SymLinkMap.end()) {
-if (!real_path(Dir, RealPath))
+if (llvm::sys::fs::real_path(Dir, RealPath))
return false;
SymLinkMap[Dir] = RealPath.str();
} else {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen
aaron.ballman added inline comments.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:21
+
+static std::string exprToStr(const Expr *Expr,
+ const MatchFinder::MatchResult &Result) {
Do not name the parameter with the same name as a type. Same applies elsewhere
in this file.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:21
+
+static std::string exprToStr(const Expr *Expr,
+ const MatchFinder::MatchResult &Result) {
aaron.ballman wrote:
> Do not name the parameter with the same name as a type. Same applies
> elsewhere in this file.
Why returning a `std::string` rather than a `StringRef`? It seems like the
callers don't always need the extra copy operation.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:36
+const MatchFinder::MatchResult &Result) {
+ auto NewExpr = const_cast(Expr);
+ ParenExpr *PE = new (Result.Context)
Can you sink the `const_cast` into the only place it's required, and remove the
`clang::` from it?
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:40
+NewExpr->getLocEnd().getLocWithOffset(1), NewExpr);
+ return Expr == PE->getSubExpr();
+}
This seems really inefficient -- it's creating a new `ParentExpr` just to throw
it away each time. Also, I'm not certain what it's accomplishing. The
constructor for `ParenExpr` accepts an `Expr *` (in this case, `Expr`) which it
returns from `ParenExpr::getSubExpr()`, so this looks like it's a noop that
just tests `Expr == Expr`. What have I missed?
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:70
+ const auto LengthCall =
+ callExpr(callee(functionDecl(hasAnyName("strlen", "wcslen"))),
+ expr().bind("length-call"));
Please match on `::strlen` and `::wcslen` so that you don't accidentally catch
random other function declarations.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:132-133
+
+ // clang-format off
+ const auto Alloca = Matcher("alloca", 1, 0, StrLenKind::WithoutInc);
+ const auto Calloc = Matcher("calloc", 2, 0, StrLenKind::WithoutInc);
Please use the fully-qualified names here as well. Also, I'm not keen on
needing to shut off clang-format just to get the alignment to work below -- we
don't typically try to keep things like this aligned unless there's a reason to
need it (like making the code maintainable), which I don't think applies here.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:193
+ diag(Result.Nodes.getNodeAs("expr")->getLocStart(),
+ "'%0' function's allocated memory cannot hold the null-terminator")
+ << Name;
`null terminator` (remove the hyphen).
Also, it might be nice to reformulate to drop the possessive apostrophe on
"function's". Perhaps something like: `"memory allocated by '%0' is
insufficient to hold the null terminator"`
Same elsewhere in this file.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:210
+auto Diag = diag(Result.Nodes.getNodeAs("expr")->getLocStart(),
+ "'%0' function's result is not null-terminated")
+<< Name;
Similar here: `"the result from calling '%0' is not null-terminated"` (or
something along those lines). Same elsewhere.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:226
+DiagnosticBuilder &Diag) {
+ if (getLangOpts().CPlusPlus11) {
+StringRef NewFuncName = (Name[0] != 'w') ? "strncpy_s" : "wcsncpy_s";
What about C?
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:306-307
+ auto Diag = diag(Result.Nodes.getNodeAs("expr")->getLocStart(),
+ "'strerror_s' function's result is not null-terminated and "
+ "missing the last character of the error message");
+ lengthArgInsertIncByOne(1, Result, Diag);
Similar rewording here to remove the possessive.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:321
+auto Diag = diag(FuncExpr->getLocStart(),
+ "'%0' function's comparison's length is too long")
+<< Name;
Given that this is a concern about the argument to the comparison function, I
think this diagnostic should point to the argument rather than the function,
and then it can drop the name of the function entirely to alleviate the awkward
wording. Something like `"comparison length is too long and might lead to a
buffer overflow"`.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.h:59
+ void
+
[PATCH] D43578: -ftime-report switch support in Clang
thakis added a comment. Also, please add cfe-commits to clang changes. Since this wasn't here and the patch wasn't seen by clang folks, since ftime-report-template-decl.cppwarnings is still failing on the bots (e.g. http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16388) and since it breaks the shared bot, maybe we should revert for now and reland when the issues are addressed? Repository: rL LLVM https://reviews.llvm.org/D43578 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45479: [Tooling] Optimize memory usage in InMemoryToolResults.
sammccall added inline comments.
Comment at: include/clang/Tooling/Execution.h:55
class InMemoryToolResults : public ToolResults {
public:
please add a high-level comment explaining what this is caching and when we
expect it to work well.
Repository:
rC Clang
https://reviews.llvm.org/D45479
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44932: [CodeComplete] Fix completion in the middle of ident in ctor lists.
aaron.ballman added a reviewer: rsmith. aaron.ballman added inline comments. Comment at: lib/Lex/Lexer.cpp:1667-1668 + assert(CurPtr < BufferEnd && "eof at completion point"); + while (isIdentifierBody(*CurPtr)) +++CurPtr; + BufferPtr = CurPtr; You should continue to assert that `CurPtr < BufferEnd` in this loop, no? Repository: rC Clang https://reviews.llvm.org/D44932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29768: [TargetInfo] Set 'UseSignedCharForObjCBool' to false by default
aaron.ballman added inline comments.
Comment at: lib/Basic/Targets/PPC.h:349
LongLongAlign = 32;
+UseSignedCharForObjCBool = true;
resetDataLayout("E-m:o-p:32:32-f64:32:64-n32");
Do you need to specify this? Isn't it handled by the `TargetInfo` constructor?
Comment at: lib/Basic/Targets/PPC.h:364
HasAlignMac68kSupport = true;
+UseSignedCharForObjCBool = true;
resetDataLayout("E-m:o-i64:64-n32:64");
Likewise.
https://reviews.llvm.org/D29768
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r329689 - CodeGen tests - typo fixes NFC
Author: gbuella
Date: Tue Apr 10 04:20:05 2018
New Revision: 329689
URL: http://llvm.org/viewvc/llvm-project?rev=329689&view=rev
Log:
CodeGen tests - typo fixes NFC
Modified:
cfe/trunk/test/CodeGen/avx512-reduceMinMaxIntrin.c
cfe/trunk/test/CodeGen/avx512f-builtins.c
Modified: cfe/trunk/test/CodeGen/avx512-reduceMinMaxIntrin.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/avx512-reduceMinMaxIntrin.c?rev=329689&r1=329688&r2=329689&view=diff
==
--- cfe/trunk/test/CodeGen/avx512-reduceMinMaxIntrin.c (original)
+++ cfe/trunk/test/CodeGen/avx512-reduceMinMaxIntrin.c Tue Apr 10 04:20:05 2018
@@ -227,7 +227,7 @@ double test_mm512_reduce_max_pd(__m512d
// CHECK: [[TMP6:%.*]] = load <8 x i64>, <8 x i64>* [[__B_ADDR_I_I]], align
64
// CHECK: store <8 x i64> zeroinitializer, <8 x i64>*
[[_COMPOUNDLITERAL_I_I_I]], align 64
// CHECK: [[TMP7:%.*]] = load <8 x i64>, <8 x i64>*
[[_COMPOUNDLITERAL_I_I_I]], align 64
-// CHECK: [[TMP8:%.*]] = icmp sgt <8 x i64> [[TMP5]], [[TMP6]]
+// CHECK: [[TMP8:%.*]] = icmp slt <8 x i64> [[TMP5]], [[TMP6]]
// CHECK: [[TMP9:%.*]] = select <8 x i1> [[TMP8]], <8 x i64> [[TMP5]], <8 x
i64> [[TMP6]]
// CHECK: store <8 x i64> [[TMP9]], <8 x i64>* [[__V_ADDR_I]], align 64
// CHECK: [[TMP10:%.*]] = load <8 x i64>, <8 x i64>* [[__V_ADDR_I]], align 64
@@ -242,7 +242,7 @@ double test_mm512_reduce_max_pd(__m512d
// CHECK: [[TMP15:%.*]] = load <8 x i64>, <8 x i64>* [[__B_ADDR_I13_I]],
align 64
// CHECK: store <8 x i64> zeroinitializer, <8 x i64>*
[[_COMPOUNDLITERAL_I_I11_I]], align 64
// CHECK: [[TMP16:%.*]] = load <8 x i64>, <8 x i64>*
[[_COMPOUNDLITERAL_I_I11_I]], align 64
-// CHECK: [[TMP17:%.*]] = icmp sgt <8 x i64> [[TMP14]], [[TMP15]]
+// CHECK: [[TMP17:%.*]] = icmp slt <8 x i64> [[TMP14]], [[TMP15]]
// CHECK: [[TMP18:%.*]] = select <8 x i1> [[TMP17]], <8 x i64> [[TMP14]], <8
x i64> [[TMP15]]
// CHECK: store <8 x i64> [[TMP18]], <8 x i64>* [[__V_ADDR_I]], align 64
// CHECK: [[TMP19:%.*]] = load <8 x i64>, <8 x i64>* [[__V_ADDR_I]], align 64
@@ -257,14 +257,14 @@ double test_mm512_reduce_max_pd(__m512d
// CHECK: [[TMP24:%.*]] = load <8 x i64>, <8 x i64>* [[__B_ADDR_I10_I]],
align 64
// CHECK: store <8 x i64> zeroinitializer, <8 x i64>*
[[_COMPOUNDLITERAL_I_I8_I]], align 64
// CHECK: [[TMP25:%.*]] = load <8 x i64>, <8 x i64>*
[[_COMPOUNDLITERAL_I_I8_I]], align 64
-// CHECK: [[TMP26:%.*]] = icmp sgt <8 x i64> [[TMP23]], [[TMP24]]
+// CHECK: [[TMP26:%.*]] = icmp slt <8 x i64> [[TMP23]], [[TMP24]]
// CHECK: [[TMP27:%.*]] = select <8 x i1> [[TMP26]], <8 x i64> [[TMP23]], <8
x i64> [[TMP24]]
// CHECK: store <8 x i64> [[TMP27]], <8 x i64>* [[__V_ADDR_I]], align 64
// CHECK: [[TMP28:%.*]] = load <8 x i64>, <8 x i64>* [[__V_ADDR_I]], align 64
// CHECK: [[VECEXT_I:%.*]] = extractelement <8 x i64> [[TMP28]], i32 0
// CHECK: ret i64 [[VECEXT_I]]
long long test_mm512_reduce_min_epi64(__m512i __W){
- return _mm512_reduce_max_epi64(__W);
+ return _mm512_reduce_min_epi64(__W);
}
// CHECK-LABEL: define i64 @test_mm512_reduce_min_epu64(<8 x i64> %__W) #0 {
@@ -294,7 +294,7 @@ long long test_mm512_reduce_min_epi64(__
// CHECK: [[TMP6:%.*]] = load <8 x i64>, <8 x i64>* [[__B_ADDR_I_I]], align
64
// CHECK: store <8 x i64> zeroinitializer, <8 x i64>*
[[_COMPOUNDLITERAL_I_I_I]], align 64
// CHECK: [[TMP7:%.*]] = load <8 x i64>, <8 x i64>*
[[_COMPOUNDLITERAL_I_I_I]], align 64
-// CHECK: [[TMP8:%.*]] = icmp ugt <8 x i64> [[TMP5]], [[TMP6]]
+// CHECK: [[TMP8:%.*]] = icmp ult <8 x i64> [[TMP5]], [[TMP6]]
// CHECK: [[TMP9:%.*]] = select <8 x i1> [[TMP8]], <8 x i64> [[TMP5]], <8 x
i64> [[TMP6]]
// CHECK: store <8 x i64> [[TMP9]], <8 x i64>* [[__V_ADDR_I]], align 64
// CHECK: [[TMP10:%.*]] = load <8 x i64>, <8 x i64>* [[__V_ADDR_I]], align 64
@@ -309,7 +309,7 @@ long long test_mm512_reduce_min_epi64(__
// CHECK: [[TMP15:%.*]] = load <8 x i64>, <8 x i64>* [[__B_ADDR_I13_I]],
align 64
// CHECK: store <8 x i64> zeroinitializer, <8 x i64>*
[[_COMPOUNDLITERAL_I_I11_I]], align 64
// CHECK: [[TMP16:%.*]] = load <8 x i64>, <8 x i64>*
[[_COMPOUNDLITERAL_I_I11_I]], align 64
-// CHECK: [[TMP17:%.*]] = icmp ugt <8 x i64> [[TMP14]], [[TMP15]]
+// CHECK: [[TMP17:%.*]] = icmp ult <8 x i64> [[TMP14]], [[TMP15]]
// CHECK: [[TMP18:%.*]] = select <8 x i1> [[TMP17]], <8 x i64> [[TMP14]], <8
x i64> [[TMP15]]
// CHECK: store <8 x i64> [[TMP18]], <8 x i64>* [[__V_ADDR_I]], align 64
// CHECK: [[TMP19:%.*]] = load <8 x i64>, <8 x i64>* [[__V_ADDR_I]], align 64
@@ -324,14 +324,14 @@ long long test_mm512_reduce_min_epi64(__
// CHECK: [[TMP24:%.*]] = load <8 x i64>, <8 x i64>* [[__B_ADDR_I10_I]],
align 64
// CHECK: store <8 x i64> zeroinitializer, <8 x i64>*
[[_COMPOUNDLITERAL_I_I8_I]], align 64
// CHECK: [[TMP25:%.*]] = load <8 x i64>, <8 x i64>*
[[_COMPOUNDLITERAL_I_I8_I]], align 64
-// CHECK:
[PATCH] D45483: [NEON] Support vfma_n and vfms_n intrinsics
t.p.northover accepted this revision. t.p.northover added a comment. This revision is now accepted and ready to land. Looks fine to me. https://reviews.llvm.org/D45483 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45485: [CMake] Unbreak BUILD_SHARED_LIBS workflow
davezarzycki added a comment. Great! I'll revisit this patch in a week and see if it is still needed. Hopefully the author of https://reviews.llvm.org/D43578 will make this patch unnecessary by then. Repository: rC Clang https://reviews.llvm.org/D45485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.
alexfh added a comment. In https://reviews.llvm.org/D38455#1062722, @courbet wrote: > In https://reviews.llvm.org/D38455#1062718, @JonasToth wrote: > > > > Sure. Is it OK to add a dependency from > > > clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in > > > clang-tidy/cpp-coreguidelines ? Is there an existing example of this ? > > > > Take a look in the hicpp module, almost everything there is aliased :) > > > Thanks for the pointer ! We currently don't have any strict rules about aliases, but we should remember that aliases have certain costs (in terms of maintenance and possible user-facing issues like duplicate warnings from different checks). I think that in general it would be better to avoid excessive aliases. There are cases when aliases are inevitable (or at least feel like a proper solution), like when multiple style guides have the same rule or very similar rules that make sense to be implemented in a single check with a bit of configurability to easily accommodate the differences. But in this case there's so far one specific rule of the C++ Core Guidelines that the check is enforcing, and adding an alias under bugprone- for it seems to be unnecessary. I also think that if a user wants to enable this check, they'd better know that it backs up a rule of the C++ Core Guidelines rather than being just an invention of clang-tidy contributors (there's nothing wrong with the latter, it's just nice to give credits where appropriate ;). I suggest to try applying the following decision process to find proper place(s) for a check: 1. List all use cases we want to support, decide whether customizations (via check options) will be needed to support them. Use case may be - enforcement of a rule of a certain style guide a clang-tidy module for which exists or is being added together with the check; - generic use case not covered by a rule of a style guide. These only count if there is no "style guide" use case that doesn't require customizations. 2. If the check only has a single "style guide" use case, place it to the corresponding module. 3. If a check (without customizations) implements rules of two or more style guides, and dependency structure allows, place it to one of the corresponding modules and add aliases to the others. 4. Otherwise choose a "generic" module for the check (bugprone-, readability-, modernize-, performance-, portability-, or misc-, if the check doesn't suit a more specific module). Add aliases to the "style guide" modules, if needed. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
thakis added a comment. This landing made our clang trunk bots do an evaluation of this warning :-P It fired 8 times, all false positives, and all from unit tests testing that operator= works for self-assignment. (https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has the exact details) It looks like the same issue exists in LLVM itself too, https://reviews.llvm.org/D45082 Now tests often need warning suppressions for things like this, and this in itself doesn't seem horrible. However, this change takes a warning that was previously 100% noise-free in practice and makes it pretty noisy – without a big benefit in practice. I get that it's beneficial in theory, but that's true of many warnings. Based on how this warning does in practice, I think it might be better for the static analyzer, which has a lower bar for false positives. Repository: rC Clang https://reviews.llvm.org/D44883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45058: [X86] Disable SGX for Skylake Server
This revision was automatically updated to reflect the committed changes. Closed by commit rC329701: [X86] Disable SGX for Skylake Server (authored by GBuella, committed by ). Changed prior to commit: https://reviews.llvm.org/D45058?vs=140302&id=141839#toc Repository: rL LLVM https://reviews.llvm.org/D45058 Files: lib/Basic/Targets/X86.cpp Index: lib/Basic/Targets/X86.cpp === --- lib/Basic/Targets/X86.cpp +++ lib/Basic/Targets/X86.cpp @@ -182,7 +182,8 @@ setFeatureEnabledImpl(Features, "xsavec", true); setFeatureEnabledImpl(Features, "xsaves", true); setFeatureEnabledImpl(Features, "mpx", true); -setFeatureEnabledImpl(Features, "sgx", true); +if (Kind != CK_SkylakeServer) // SKX inherits all SKL features, except SGX + setFeatureEnabledImpl(Features, "sgx", true); setFeatureEnabledImpl(Features, "clflushopt", true); setFeatureEnabledImpl(Features, "rtm", true); LLVM_FALLTHROUGH; Index: lib/Basic/Targets/X86.cpp === --- lib/Basic/Targets/X86.cpp +++ lib/Basic/Targets/X86.cpp @@ -182,7 +182,8 @@ setFeatureEnabledImpl(Features, "xsavec", true); setFeatureEnabledImpl(Features, "xsaves", true); setFeatureEnabledImpl(Features, "mpx", true); -setFeatureEnabledImpl(Features, "sgx", true); +if (Kind != CK_SkylakeServer) // SKX inherits all SKL features, except SGX + setFeatureEnabledImpl(Features, "sgx", true); setFeatureEnabledImpl(Features, "clflushopt", true); setFeatureEnabledImpl(Features, "rtm", true); LLVM_FALLTHROUGH; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45058: [X86] Disable SGX for Skylake Server
This revision was automatically updated to reflect the committed changes. Closed by commit rL329701: [X86] Disable SGX for Skylake Server (authored by GBuella, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D45058 Files: cfe/trunk/lib/Basic/Targets/X86.cpp Index: cfe/trunk/lib/Basic/Targets/X86.cpp === --- cfe/trunk/lib/Basic/Targets/X86.cpp +++ cfe/trunk/lib/Basic/Targets/X86.cpp @@ -182,7 +182,8 @@ setFeatureEnabledImpl(Features, "xsavec", true); setFeatureEnabledImpl(Features, "xsaves", true); setFeatureEnabledImpl(Features, "mpx", true); -setFeatureEnabledImpl(Features, "sgx", true); +if (Kind != CK_SkylakeServer) // SKX inherits all SKL features, except SGX + setFeatureEnabledImpl(Features, "sgx", true); setFeatureEnabledImpl(Features, "clflushopt", true); setFeatureEnabledImpl(Features, "rtm", true); LLVM_FALLTHROUGH; Index: cfe/trunk/lib/Basic/Targets/X86.cpp === --- cfe/trunk/lib/Basic/Targets/X86.cpp +++ cfe/trunk/lib/Basic/Targets/X86.cpp @@ -182,7 +182,8 @@ setFeatureEnabledImpl(Features, "xsavec", true); setFeatureEnabledImpl(Features, "xsaves", true); setFeatureEnabledImpl(Features, "mpx", true); -setFeatureEnabledImpl(Features, "sgx", true); +if (Kind != CK_SkylakeServer) // SKX inherits all SKL features, except SGX + setFeatureEnabledImpl(Features, "sgx", true); setFeatureEnabledImpl(Features, "clflushopt", true); setFeatureEnabledImpl(Features, "rtm", true); LLVM_FALLTHROUGH; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r329702 - Attempt to fix Windows build after r329698.
Author: nico Date: Tue Apr 10 07:08:25 2018 New Revision: 329702 URL: http://llvm.org/viewvc/llvm-project?rev=329702&view=rev Log: Attempt to fix Windows build after r329698. Modified: cfe/trunk/lib/Basic/FileManager.cpp Modified: cfe/trunk/lib/Basic/FileManager.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=329702&r1=329701&r2=329702&view=diff == --- cfe/trunk/lib/Basic/FileManager.cpp (original) +++ cfe/trunk/lib/Basic/FileManager.cpp Tue Apr 10 07:08:25 2018 @@ -534,7 +534,7 @@ StringRef FileManager::getCanonicalName( StringRef CanonicalName(Dir->getName()); - SmallString CanonicalNameBuf; + SmallString<256> CanonicalNameBuf; if (!llvm::sys::fs::real_path(Dir->getName(), CanonicalNameBuf)) CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos
obfuscated marked an inline comment as done. obfuscated added a comment. Ping? https://reviews.llvm.org/D44765 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1063003, @thakis wrote: > This landing made our clang trunk bots do an evaluation of this warning :-P > It fired 8 times, all false positives, and all from unit tests testing that > operator= works for self-assignment. > (https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has the > exact details) It looks like the same issue exists in LLVM itself too, > https://reviews.llvm.org/D45082 Right, i guess i only built the chrome binary itself, not the tests, good to know... > Now tests often need warning suppressions for things like this, and this in > itself doesn't seem horrible. However, this change takes a warning that was > previously 100% noise-free in practice and makes it pretty noisy – without a > big benefit in practice. I get that it's beneficial in theory, but that's > true of many warnings. > > Based on how this warning does in practice, I think it might be better for > the static analyzer, which has a lower bar for false positives. Noisy in the sense that it correctly diagnoses a self-assignment where one **intended** to have self-assignment. And unsurprisingly, it happened in the unit-tests, as was expected ^ in previous comments. **So far** there are no truly false-positives noise (at least no reports of it). We could help workaround that the way i initially suggested, by keeping this new part of the diag under it's own sub-flag, and suggest to disable it for tests. But yes, that Repository: rC Clang https://reviews.llvm.org/D44883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45479: [Tooling] Optimize memory usage in InMemoryToolResults.
hokein updated this revision to Diff 141842.
hokein edited the summary of this revision.
hokein added a comment.
Add a comment.
Repository:
rC Clang
https://reviews.llvm.org/D45479
Files:
include/clang/Tooling/Execution.h
lib/Tooling/AllTUsExecution.cpp
lib/Tooling/Execution.cpp
Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,10 +21,19 @@
llvm::cl::init("standalone"));
void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
- KVResults.push_back({Key.str(), Value.str()});
+ auto Intern = [&](StringRef &V) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+ *R.first = StringsPool.save(V);
+}
+V = *R.first;
+ };
+ Intern(Key);
+ Intern(Value);
+ KVResults.push_back({Key, Value});
}
-std::vector>
+std::vector>
InMemoryToolResults::AllKVResults() {
return KVResults;
}
Index: lib/Tooling/AllTUsExecution.cpp
===
--- lib/Tooling/AllTUsExecution.cpp
+++ lib/Tooling/AllTUsExecution.cpp
@@ -36,7 +36,8 @@
Results.addResult(Key, Value);
}
- std::vector> AllKVResults() override {
+ std::vector>
+ AllKVResults() override {
return Results.AllKVResults();
}
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
namespace clang {
namespace tooling {
@@ -45,20 +46,30 @@
public:
virtual ~ToolResults() = default;
virtual void addResult(StringRef Key, StringRef Value) = 0;
- virtual std::vector> AllKVResults() = 0;
+ virtual std::vector>
+ AllKVResults() = 0;
virtual void forEachResult(
llvm::function_ref Callback) = 0;
};
+/// \brief Stores the key-value results in memory. It maintains the lifetime of
+/// the result. Clang tools using this class are expected to generate a small
+/// set of different results, or a large set of duplicated results.
class InMemoryToolResults : public ToolResults {
public:
+ InMemoryToolResults() : StringsPool(Arena) {}
void addResult(StringRef Key, StringRef Value) override;
- std::vector> AllKVResults() override;
+ std::vector>
+ AllKVResults() override;
void forEachResult(llvm::function_ref
Callback) override;
private:
- std::vector> KVResults;
+ llvm::BumpPtrAllocator Arena;
+ llvm::StringSaver StringsPool;
+ llvm::DenseSet Strings;
+
+ std::vector> KVResults;
};
/// \brief The context of an execution, including the information about
Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,10 +21,19 @@
llvm::cl::init("standalone"));
void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
- KVResults.push_back({Key.str(), Value.str()});
+ auto Intern = [&](StringRef &V) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+ *R.first = StringsPool.save(V);
+}
+V = *R.first;
+ };
+ Intern(Key);
+ Intern(Value);
+ KVResults.push_back({Key, Value});
}
-std::vector>
+std::vector>
InMemoryToolResults::AllKVResults() {
return KVResults;
}
Index: lib/Tooling/AllTUsExecution.cpp
===
--- lib/Tooling/AllTUsExecution.cpp
+++ lib/Tooling/AllTUsExecution.cpp
@@ -36,7 +36,8 @@
Results.addResult(Key, Value);
}
- std::vector> AllKVResults() override {
+ std::vector>
+ AllKVResults() override {
return Results.AllKVResults();
}
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
namespace clang {
namespace tooling {
@@ -45,20 +46,30 @@
public:
virtual ~ToolResults() = default;
virtual void addResult(StringRef Key, StringRef Value) = 0;
- virtual std::vector> AllKVResults() = 0;
+ virtual std::vector>
+ AllKVResults() = 0;
virtual void forEachResult(
llvm::function_ref Callback) = 0;
};
+/// \brief Stores the key-value results in memory. It maintains the lifetime of
+/// the result. Clang tools using this class are expected to generate a small
+/// set of different results, or a large set of duplicated results.
class InMemoryToolResults : public ToolResults {
public:
+ InMemoryToolResults() : StringsPool(A
[PATCH] D44764: [clangd] Use operator<< to prevent printers issues in Gtest
sammccall accepted this revision. sammccall added a comment. Sorry I lost track of this. LGTM! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45058: [X86] Disable SGX for Skylake Server
davezarzycki added a comment. I think this change is incomplete. The following clang test fails on my SKX machine: `Preprocessor/predefined-arch-macros.c` Line 895: `// CHECK_SKX_M32: #define __SGX__ 1` Repository: rL LLVM https://reviews.llvm.org/D45058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43667: [clang-doc] Implement a YAML generator
juliehockett added a comment. In https://reviews.llvm.org/D43667#1062746, @Athosvk wrote: > I'm a bit late on this, but I'd say that YAML is usually not a 'final' > format. What would be the use-cases for this? And if is meant as an > alternative intermediate format, why not instead of having one big file, have > one file per input file? Just wondering what the particular motivation for > that could be The idea is that it's a generally-consumable output format, and so could be interpreted by external software fairly trivially. The bitsream format is compact and good for things that will live entirely in the clang-doc tool, but is harder to deal with outside that scope. YAML bridges that gap. I haven't thought too much about splitting it up, but it might make sense, given that the one file could get very large. By file might work--let me play with it a bit to see what makes the most sense. https://reviews.llvm.org/D43667 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45488: [X86] Disable SGX for Skylake Server - CPP test
GBuella created this revision. GBuella added reviewers: craig.topper, davezarzycki. Herald added a subscriber: cfe-commits. Fix test case - corresponding to r329701 Repository: rC Clang https://reviews.llvm.org/D45488 Files: test/Preprocessor/predefined-arch-macros.c Index: test/Preprocessor/predefined-arch-macros.c === --- test/Preprocessor/predefined-arch-macros.c +++ test/Preprocessor/predefined-arch-macros.c @@ -663,7 +663,7 @@ // CHECK_SKL_M32: #define __RDRND__ 1 // CHECK_SKL_M32: #define __RDSEED__ 1 // CHECK_SKL_M32: #define __RTM__ 1 -// CHECK_SKL_M32: #define __SGX__ 1 +// CHECK_SKL_M32-NOT: #define __SGX__ 1 // CHECK_SKL_M32: #define __SSE2__ 1 // CHECK_SKL_M32: #define __SSE3__ 1 // CHECK_SKL_M32: #define __SSE4_1__ 1 @@ -697,7 +697,7 @@ // CHECK_SKL_M64: #define __RDRND__ 1 // CHECK_SKL_M64: #define __RDSEED__ 1 // CHECK_SKL_M64: #define __RTM__ 1 -// CHECK_SKL_M64: #define __SGX__ 1 +// CHECK_SKL_M64-NOT: #define __SGX__ 1 // CHECK_SKL_M64: #define __SSE2_MATH__ 1 // CHECK_SKL_M64: #define __SSE2__ 1 // CHECK_SKL_M64: #define __SSE3__ 1 Index: test/Preprocessor/predefined-arch-macros.c === --- test/Preprocessor/predefined-arch-macros.c +++ test/Preprocessor/predefined-arch-macros.c @@ -663,7 +663,7 @@ // CHECK_SKL_M32: #define __RDRND__ 1 // CHECK_SKL_M32: #define __RDSEED__ 1 // CHECK_SKL_M32: #define __RTM__ 1 -// CHECK_SKL_M32: #define __SGX__ 1 +// CHECK_SKL_M32-NOT: #define __SGX__ 1 // CHECK_SKL_M32: #define __SSE2__ 1 // CHECK_SKL_M32: #define __SSE3__ 1 // CHECK_SKL_M32: #define __SSE4_1__ 1 @@ -697,7 +697,7 @@ // CHECK_SKL_M64: #define __RDRND__ 1 // CHECK_SKL_M64: #define __RDSEED__ 1 // CHECK_SKL_M64: #define __RTM__ 1 -// CHECK_SKL_M64: #define __SGX__ 1 +// CHECK_SKL_M64-NOT: #define __SGX__ 1 // CHECK_SKL_M64: #define __SSE2_MATH__ 1 // CHECK_SKL_M64: #define __SSE2__ 1 // CHECK_SKL_M64: #define __SSE3__ 1 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
On Tue, Apr 10, 2018 at 7:21 AM Roman Lebedev via Phabricator < revi...@reviews.llvm.org> wrote: > lebedev.ri added a comment. > > In https://reviews.llvm.org/D44883#1063003, @thakis wrote: > > > This landing made our clang trunk bots do an evaluation of this warning > :-P It fired 8 times, all false positives, and all from unit tests testing > that operator= works for self-assignment. ( > https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has the > exact details) It looks like the same issue exists in LLVM itself too, > https://reviews.llvm.org/D45082 > > > Right, i guess i only built the chrome binary itself, not the tests, good > to know... > > > Now tests often need warning suppressions for things like this, and this > in itself doesn't seem horrible. However, this change takes a warning that > was previously 100% noise-free in practice and makes it pretty noisy – > without a big benefit in practice. I get that it's beneficial in theory, > but that's true of many warnings. > > > > Based on how this warning does in practice, I think it might be better > for the static analyzer, which has a lower bar for false positives. > > Noisy in the sense that it correctly diagnoses a self-assignment where one > **intended** to have self-assignment. > And unsurprisingly, it happened in the unit-tests, as was expected ^ in > previous comments. > **So far** there are no truly false-positives noise (at least no reports > of it). > False positive in the broad sense (which I think I mentioned in review of this patch) of "diagnosing code that is working as intended (rather than diagnosing/identifying bugs/mistakes) & is fairly unsurprising/legible (ie: the warning didn't find places that weren't bugs, but were sufficiently confusing that they could be written better some other way)". In the past, the bar for warnings has been higher in my experience (though an interesting argument about whether making a change to an existing warning, versus adding a warning under a separate flag, changes the decision (since it may wash out new false negatives with the many true negatives in the original implementation)). Probably one of the best ways to evaluate whether this is beneficial in practice is to leave it in (at least inside Google or anywhere else that can track diagnostic frequency) for a while & see how often it comes up in practice... - though I don't know if we'd be able to separate out the cases that come from this improvement versus all the rest - maybe if the text is (or was made to be) worded slightly differently, or if it were under a separate flag. (but yes, I recall the discussion about flag naming and subsetting the flags, etc - I don't have a good answer to that) > We could help workaround that the way i initially suggested, by keeping > this new part of the diag under it's own sub-flag, > and suggest to disable it for tests. But yes, that > > > Repository: > rC Clang > > https://reviews.llvm.org/D44883 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45447: [clang-tidy] Adding alias for anon namespaces in header (fuchsia module)
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor docs nit. Comment at: docs/clang-tidy/checks/fuchsia-header-anon-namespaces.rst:6 +fuchsia-header-anon-namespaces += + Underlining is the wrong length. https://reviews.llvm.org/D45447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45488: [X86] Disable SGX for Skylake Server - CPP test
davezarzycki added a comment. I don't think this change is correct. To the best of my knowledge, SKL does support SGX but SKX does not. Repository: rC Clang https://reviews.llvm.org/D45488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45489: [HIP] Add input type for HIP
yaxunl created this revision.
yaxunl added reviewers: tra, Hahnfeld.
This patch is split from https://reviews.llvm.org/D45212
https://reviews.llvm.org/D45489
Files:
include/clang/Driver/Types.def
lib/Driver/Driver.cpp
lib/Driver/Types.cpp
Index: lib/Driver/Types.cpp
===
--- lib/Driver/Types.cpp
+++ lib/Driver/Types.cpp
@@ -102,6 +102,9 @@
case TY_CL:
case TY_CUDA: case TY_PP_CUDA:
case TY_CUDA_DEVICE:
+ case TY_HIP:
+ case TY_PP_HIP:
+ case TY_HIP_DEVICE:
case TY_ObjC: case TY_PP_ObjC: case TY_PP_ObjC_Alias:
case TY_CXX: case TY_PP_CXX:
case TY_ObjCXX: case TY_PP_ObjCXX: case TY_PP_ObjCXX_Alias:
@@ -141,6 +144,9 @@
case TY_ObjCXXHeader: case TY_PP_ObjCXXHeader:
case TY_CXXModule: case TY_PP_CXXModule:
case TY_CUDA: case TY_PP_CUDA: case TY_CUDA_DEVICE:
+ case TY_HIP:
+ case TY_PP_HIP:
+ case TY_HIP_DEVICE:
return true;
}
}
@@ -166,6 +172,9 @@
case TY_CUDA:
case TY_PP_CUDA:
case TY_CUDA_DEVICE:
+ case TY_HIP:
+ case TY_PP_HIP:
+ case TY_HIP_DEVICE:
return true;
}
}
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -2249,9 +2249,10 @@
assert(!GpuArchList.empty() &&
"We should have at least one GPU architecture.");
-// If the host input is not CUDA, we don't need to bother about this
-// input.
-if (IA->getType() != types::TY_CUDA) {
+// If the host input is not CUDA or HIP, we don't need to bother about
+// this input.
+if (IA->getType() != types::TY_CUDA &&
+IA->getType() != types::TY_HIP) {
// The builder will ignore this input.
IsActive = false;
return ABRT_Inactive;
@@ -2264,9 +2265,12 @@
return ABRT_Success;
// Replicate inputs for each GPU architecture.
-for (unsigned I = 0, E = GpuArchList.size(); I != E; ++I)
- CudaDeviceActions.push_back(C.MakeAction(
- IA->getInputArg(), types::TY_CUDA_DEVICE));
+auto Ty = IA->getType() == types::TY_HIP ? types::TY_HIP_DEVICE
+ : types::TY_CUDA_DEVICE;
+for (unsigned I = 0, E = GpuArchList.size(); I != E; ++I) {
+ CudaDeviceActions.push_back(
+ C.MakeAction(IA->getInputArg(), Ty));
+}
return ABRT_Success;
}
Index: include/clang/Driver/Types.def
===
--- include/clang/Driver/Types.def
+++ include/clang/Driver/Types.def
@@ -46,6 +46,9 @@
TYPE("cuda-cpp-output", PP_CUDA, INVALID, "cui", "u")
TYPE("cuda", CUDA, PP_CUDA, "cu","u")
TYPE("cuda", CUDA_DEVICE, PP_CUDA, "cu","")
+TYPE("hip-cpp-output", PP_HIP, INVALID, "cui", "u")
+TYPE("hip", HIP, PP_HIP, "cu","u")
+TYPE("hip", HIP_DEVICE, PP_HIP, "cu","")
TYPE("objective-c-cpp-output", PP_ObjC, INVALID, "mi","u")
TYPE("objc-cpp-output", PP_ObjC_Alias, INVALID,"mi","u")
TYPE("objective-c", ObjC, PP_ObjC, "m", "u")
Index: lib/Driver/Types.cpp
===
--- lib/Driver/Types.cpp
+++ lib/Driver/Types.cpp
@@ -102,6 +102,9 @@
case TY_CL:
case TY_CUDA: case TY_PP_CUDA:
case TY_CUDA_DEVICE:
+ case TY_HIP:
+ case TY_PP_HIP:
+ case TY_HIP_DEVICE:
case TY_ObjC: case TY_PP_ObjC: case TY_PP_ObjC_Alias:
case TY_CXX: case TY_PP_CXX:
case TY_ObjCXX: case TY_PP_ObjCXX: case TY_PP_ObjCXX_Alias:
@@ -141,6 +144,9 @@
case TY_ObjCXXHeader: case TY_PP_ObjCXXHeader:
case TY_CXXModule: case TY_PP_CXXModule:
case TY_CUDA: case TY_PP_CUDA: case TY_CUDA_DEVICE:
+ case TY_HIP:
+ case TY_PP_HIP:
+ case TY_HIP_DEVICE:
return true;
}
}
@@ -166,6 +172,9 @@
case TY_CUDA:
case TY_PP_CUDA:
case TY_CUDA_DEVICE:
+ case TY_HIP:
+ case TY_PP_HIP:
+ case TY_HIP_DEVICE:
return true;
}
}
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -2249,9 +2249,10 @@
assert(!GpuArchList.empty() &&
"We should have at least one GPU architecture.");
-// If the host input is not CUDA, we don't need to bother about this
-// input.
-if (IA->getType() != types::TY_CUDA) {
+// If the host input is not CUDA or HIP, we don't need to bother about
+// this input.
+if (IA->getType() != types::TY_CUDA &&
+IA->getType() != types::TY_HIP) {
// The builder will ignore this input.
IsActive = false;
retur
[PATCH] D45488: [X86] Disable SGX for Skylake Server - CPP test
GBuella updated this revision to Diff 141853. https://reviews.llvm.org/D45488 Files: test/Preprocessor/predefined-arch-macros.c Index: test/Preprocessor/predefined-arch-macros.c === --- test/Preprocessor/predefined-arch-macros.c +++ test/Preprocessor/predefined-arch-macros.c @@ -892,7 +892,7 @@ // CHECK_SKX_M32: #define __RDRND__ 1 // CHECK_SKX_M32: #define __RDSEED__ 1 // CHECK_SKX_M32: #define __RTM__ 1 -// CHECK_SKX_M32: #define __SGX__ 1 +// CHECK_SKX_M32-NOT: #define __SGX__ 1 // CHECK_SKX_M32: #define __SSE2__ 1 // CHECK_SKX_M32: #define __SSE3__ 1 // CHECK_SKX_M32: #define __SSE4_1__ 1 @@ -937,7 +937,7 @@ // CHECK_SKX_M64: #define __RDRND__ 1 // CHECK_SKX_M64: #define __RDSEED__ 1 // CHECK_SKX_M64: #define __RTM__ 1 -// CHECK_SKX_M64: #define __SGX__ 1 +// CHECK_SKX_M64-NOT: #define __SGX__ 1 // CHECK_SKX_M64: #define __SSE2_MATH__ 1 // CHECK_SKX_M64: #define __SSE2__ 1 // CHECK_SKX_M64: #define __SSE3__ 1 Index: test/Preprocessor/predefined-arch-macros.c === --- test/Preprocessor/predefined-arch-macros.c +++ test/Preprocessor/predefined-arch-macros.c @@ -892,7 +892,7 @@ // CHECK_SKX_M32: #define __RDRND__ 1 // CHECK_SKX_M32: #define __RDSEED__ 1 // CHECK_SKX_M32: #define __RTM__ 1 -// CHECK_SKX_M32: #define __SGX__ 1 +// CHECK_SKX_M32-NOT: #define __SGX__ 1 // CHECK_SKX_M32: #define __SSE2__ 1 // CHECK_SKX_M32: #define __SSE3__ 1 // CHECK_SKX_M32: #define __SSE4_1__ 1 @@ -937,7 +937,7 @@ // CHECK_SKX_M64: #define __RDRND__ 1 // CHECK_SKX_M64: #define __RDSEED__ 1 // CHECK_SKX_M64: #define __RTM__ 1 -// CHECK_SKX_M64: #define __SGX__ 1 +// CHECK_SKX_M64-NOT: #define __SGX__ 1 // CHECK_SKX_M64: #define __SSE2_MATH__ 1 // CHECK_SKX_M64: #define __SSE2__ 1 // CHECK_SKX_M64: #define __SSE3__ 1 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43667: [clang-doc] Implement a YAML generator
Athosvk added a comment. In https://reviews.llvm.org/D43667#1063049, @juliehockett wrote: > In https://reviews.llvm.org/D43667#1062746, @Athosvk wrote: > > > I'm a bit late on this, but I'd say that YAML is usually not a 'final' > > format. What would be the use-cases for this? And if is meant as an > > alternative intermediate format, why not instead of having one big file, > > have one file per input file? Just wondering what the particular motivation > > for that could be > > > The idea is that it's a generally-consumable output format, and so could be > interpreted by external software fairly trivially. The bitsream format is > compact and good for things that will live entirely in the clang-doc tool, > but is harder to deal with outside that scope. YAML bridges that gap. That's what I expected :). The primary advantage of one file per output to us was granularity. That allows you to do incremental builds and distribute them at this stage (locally over multiple cores, but also remotely if need be). https://reviews.llvm.org/D43667 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM with some minor commenting nits.
Comment at: lib/CodeGen/CGBuiltin.cpp:992
+
+// We check whether we are in a recursive type
+if (CanonicalType->isRecordType()) {
Missing full stop at the end of the comment. Same elsewhere.
Comment at: lib/Sema/SemaChecking.cpp:1114
+ case Builtin::BI__builtin_dump_struct: {
+// We first want to ensure we are called with 2 arguments
+if (checkArgCount(*this, TheCall, 2))
Missing full stop here and elsewhere as well.
Repository:
rC Clang
https://reviews.llvm.org/D44093
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45488: [X86] Disable SGX for Skylake Server - CPP test
GBuella added a comment. In https://reviews.llvm.org/D45488#1063067, @davezarzycki wrote: > I don't think this change is correct. To the best of my knowledge, SKL does > support SGX but SKX does not. llvm-lit test/Preprocessor/predefined-arch-macros.c passes now. I didn't know tests are not run automatically precommit around here, now I know I must always check them manually... sorry https://reviews.llvm.org/D45488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45488: [X86] Disable SGX for Skylake Server - CPP test
This revision was automatically updated to reflect the committed changes. Closed by commit rL329710: [X86] Disable SGX for Skylake Server - CPP test (authored by GBuella, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D45488?vs=141853&id=141855#toc Repository: rL LLVM https://reviews.llvm.org/D45488 Files: cfe/trunk/test/Preprocessor/predefined-arch-macros.c Index: cfe/trunk/test/Preprocessor/predefined-arch-macros.c === --- cfe/trunk/test/Preprocessor/predefined-arch-macros.c +++ cfe/trunk/test/Preprocessor/predefined-arch-macros.c @@ -892,7 +892,7 @@ // CHECK_SKX_M32: #define __RDRND__ 1 // CHECK_SKX_M32: #define __RDSEED__ 1 // CHECK_SKX_M32: #define __RTM__ 1 -// CHECK_SKX_M32: #define __SGX__ 1 +// CHECK_SKX_M32-NOT: #define __SGX__ 1 // CHECK_SKX_M32: #define __SSE2__ 1 // CHECK_SKX_M32: #define __SSE3__ 1 // CHECK_SKX_M32: #define __SSE4_1__ 1 @@ -937,7 +937,7 @@ // CHECK_SKX_M64: #define __RDRND__ 1 // CHECK_SKX_M64: #define __RDSEED__ 1 // CHECK_SKX_M64: #define __RTM__ 1 -// CHECK_SKX_M64: #define __SGX__ 1 +// CHECK_SKX_M64-NOT: #define __SGX__ 1 // CHECK_SKX_M64: #define __SSE2_MATH__ 1 // CHECK_SKX_M64: #define __SSE2__ 1 // CHECK_SKX_M64: #define __SSE3__ 1 Index: cfe/trunk/test/Preprocessor/predefined-arch-macros.c === --- cfe/trunk/test/Preprocessor/predefined-arch-macros.c +++ cfe/trunk/test/Preprocessor/predefined-arch-macros.c @@ -892,7 +892,7 @@ // CHECK_SKX_M32: #define __RDRND__ 1 // CHECK_SKX_M32: #define __RDSEED__ 1 // CHECK_SKX_M32: #define __RTM__ 1 -// CHECK_SKX_M32: #define __SGX__ 1 +// CHECK_SKX_M32-NOT: #define __SGX__ 1 // CHECK_SKX_M32: #define __SSE2__ 1 // CHECK_SKX_M32: #define __SSE3__ 1 // CHECK_SKX_M32: #define __SSE4_1__ 1 @@ -937,7 +937,7 @@ // CHECK_SKX_M64: #define __RDRND__ 1 // CHECK_SKX_M64: #define __RDSEED__ 1 // CHECK_SKX_M64: #define __RTM__ 1 -// CHECK_SKX_M64: #define __SGX__ 1 +// CHECK_SKX_M64-NOT: #define __SGX__ 1 // CHECK_SKX_M64: #define __SSE2_MATH__ 1 // CHECK_SKX_M64: #define __SSE2__ 1 // CHECK_SKX_M64: #define __SSE3__ 1 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r329701 - [X86] Disable SGX for Skylake Server
Author: gbuella Date: Tue Apr 10 07:04:21 2018 New Revision: 329701 URL: http://llvm.org/viewvc/llvm-project?rev=329701&view=rev Log: [X86] Disable SGX for Skylake Server Reviewers: craig.topper, zvi, echristo Reviewed By: craig.topper Differential Revision: https://reviews.llvm.org/D45058 Modified: cfe/trunk/lib/Basic/Targets/X86.cpp Modified: cfe/trunk/lib/Basic/Targets/X86.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/X86.cpp?rev=329701&r1=329700&r2=329701&view=diff == --- cfe/trunk/lib/Basic/Targets/X86.cpp (original) +++ cfe/trunk/lib/Basic/Targets/X86.cpp Tue Apr 10 07:04:21 2018 @@ -182,7 +182,8 @@ bool X86TargetInfo::initFeatureMap( setFeatureEnabledImpl(Features, "xsavec", true); setFeatureEnabledImpl(Features, "xsaves", true); setFeatureEnabledImpl(Features, "mpx", true); -setFeatureEnabledImpl(Features, "sgx", true); +if (Kind != CK_SkylakeServer) // SKX inherits all SKL features, except SGX + setFeatureEnabledImpl(Features, "sgx", true); setFeatureEnabledImpl(Features, "clflushopt", true); setFeatureEnabledImpl(Features, "rtm", true); LLVM_FALLTHROUGH; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r329710 - [X86] Disable SGX for Skylake Server - CPP test
Author: gbuella Date: Tue Apr 10 08:03:03 2018 New Revision: 329710 URL: http://llvm.org/viewvc/llvm-project?rev=329710&view=rev Log: [X86] Disable SGX for Skylake Server - CPP test Summary: Fix test case - corresponding to r329701 Reviewers: craig.topper, davezarzycki Reviewed By: davezarzycki Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D45488 Modified: cfe/trunk/test/Preprocessor/predefined-arch-macros.c Modified: cfe/trunk/test/Preprocessor/predefined-arch-macros.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preprocessor/predefined-arch-macros.c?rev=329710&r1=329709&r2=329710&view=diff == --- cfe/trunk/test/Preprocessor/predefined-arch-macros.c (original) +++ cfe/trunk/test/Preprocessor/predefined-arch-macros.c Tue Apr 10 08:03:03 2018 @@ -892,7 +892,7 @@ // CHECK_SKX_M32: #define __RDRND__ 1 // CHECK_SKX_M32: #define __RDSEED__ 1 // CHECK_SKX_M32: #define __RTM__ 1 -// CHECK_SKX_M32: #define __SGX__ 1 +// CHECK_SKX_M32-NOT: #define __SGX__ 1 // CHECK_SKX_M32: #define __SSE2__ 1 // CHECK_SKX_M32: #define __SSE3__ 1 // CHECK_SKX_M32: #define __SSE4_1__ 1 @@ -937,7 +937,7 @@ // CHECK_SKX_M64: #define __RDRND__ 1 // CHECK_SKX_M64: #define __RDSEED__ 1 // CHECK_SKX_M64: #define __RTM__ 1 -// CHECK_SKX_M64: #define __SGX__ 1 +// CHECK_SKX_M64-NOT: #define __SGX__ 1 // CHECK_SKX_M64: #define __SSE2_MATH__ 1 // CHECK_SKX_M64: #define __SSE2__ 1 // CHECK_SKX_M64: #define __SSE3__ 1 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos
alexfh added inline comments.
Comment at: lib/Format/FormatTokenLexer.cpp:371
FormatToken *Macro = Tokens[Tokens.size() - 4];
- if (Macro->TokenText != "_T")
+ if (std::find(Style.TMacros.begin(), Style.TMacros.end(), Macro->TokenText)
==
+ Style.TMacros.end())
obfuscated wrote:
> alexfh wrote:
> > Consider using llvm::find, which is a range adaptor for std::find.
> No idea what is range adaptor, but I can probably switch to it... :)
It works with a range (something that has .begin() and .end()) instead of two
iterators.
Comment at: lib/Format/FormatTokenLexer.cpp:386
String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+ String->TMacroStringLiteral = true;
obfuscated wrote:
> alexfh wrote:
> > obfuscated wrote:
> > > krasimir wrote:
> > > > In the original code, TMacro detection was done as:
> > > > ```
> > > > (Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")"))
> > > > ```
> > > > In the new code the left part is saved in TMacroStringLiteral, and the
> > > > right part is checked in ContinuationIndenter. Could you keep them
> > > > together in `FormatTokenLexer`.
> > > > @alexfh, why are we checking for the right check at all? What would be
> > > > an example where this is needed to disambiguate?
> > > >
> > > Are you talking about the code in
> > > ContinuationIndenter::createBreakableToken?
> > >
> > > I don't think I understand how the hole thing works.
> > > Using the debugger I can see that this function is executed first and
> > > then createBreakableToken.
> > > So we first detect the tmacro in FormatTokenLexer::tryMerge_TMacro and
> > > then use this information in the createBreakableToken to do something
> > > with it.
> > >
> > > So when I get a TMacroStringLiteral==true in createBreakableToken I know
> > > that the token starts with a TMacro and there is no need to use some king
> > > of startswith calls. Probably the endswith call is redundant and I can do
> > > just a string search backwards...
> > > In the new code the left part is saved in TMacroStringLiteral, and the
> > > right part is checked in ContinuationIndenter. Could you keep them
> > > together in FormatTokenLexer.
> >
> > +1 to keeping these conditions together.
> >
> > > @alexfh, why are we checking for the right check at all? What would be an
> > > example where this is needed to disambiguate?
> >
> > I don't know whether there's reasonable code that would be handled
> > incorrectly without checking for the `")`, but I'm sure some test case can
> > be generated, that would break clang-format badly without this condition.
> > It also serves as a documentation (similar to asserts).
> >> In the new code the left part is saved in TMacroStringLiteral, and the
> >> right part is checked in ContinuationIndenter. Could you keep them
> >> together in FormatTokenLexer.
> >
> > +1 to keeping these conditions together.
>
> Can you please explain what you mean here?
> And what am I supposed to do, because I don't know how to put these
> conditions together.
> Do you want me to search the tmacro vector in
> ContinuationIndenter::createBreakableToken instead of checking the bool flag?
>
>
>
Looking at this code again, I see what the checks for `_T("` and `")` were
needed for. If there was a space between `_T(` and `"` or between `"` and `)`,
the code in `createBreakableToken()` would probably not work correctly (see the
FIXME). `tryMerge_TMacro()` looks at tokens and doesn't care about the
whitespace, so checking for `TMacroStringLiteral` is not equivalent to checking
for `("` and `")`. In particular, if you only remove just the first
check, a test case like `_T ( "...")` may start being formatted in some way.
We could remove the second check as well, if we manage to make formatting
decent. Otherwise, performing these checks (that there are no spaces between
`_T`, `(`, `"..."` and `)`) in `tryMerge_TMacro()` may be a better option than
doing this in `createBreakableToken()`.
https://reviews.llvm.org/D44765
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
"False positive" means "warning fires but didn't find anything interesting", not "warning fires while being technically correct". So all these instances do count as false positives. clang tries super hard to make sure that every time a warning fires, it is useful for a dev to fix it. If you build with warnings enabled, that should be a rewarding experience. Often, this means dialing back a warning to not warn in cases where it would make sense in theory when in practice the warning doesn't find much compared to the amount of noise it generates. This is why for example clang's -Woverloaded-virtual is usable while gcc's isn't (or wasn't last I checked a while ago) – gcc fires always when it's technically correct to do so, clang only when it actually matters in practice. On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via cfe-commits wrote: > lebedev.ri added a comment. > > In https://reviews.llvm.org/D44883#1063003, @thakis wrote: > > > This landing made our clang trunk bots do an evaluation of this warning > :-P It fired 8 times, all false positives, and all from unit tests testing > that operator= works for self-assignment. (https://chromium-review. > googlesource.com/c/chromium/src/+/1000856 has the exact details) It looks > like the same issue exists in LLVM itself too, https://reviews.llvm.org/ > D45082 > > > Right, i guess i only built the chrome binary itself, not the tests, good > to know... > > > Now tests often need warning suppressions for things like this, and this > in itself doesn't seem horrible. However, this change takes a warning that > was previously 100% noise-free in practice and makes it pretty noisy – > without a big benefit in practice. I get that it's beneficial in theory, > but that's true of many warnings. > > > > Based on how this warning does in practice, I think it might be better > for the static analyzer, which has a lower bar for false positives. > > Noisy in the sense that it correctly diagnoses a self-assignment where one > **intended** to have self-assignment. > And unsurprisingly, it happened in the unit-tests, as was expected ^ in > previous comments. > **So far** there are no truly false-positives noise (at least no reports > of it). > > We could help workaround that the way i initially suggested, by keeping > this new part of the diag under it's own sub-flag, > and suggest to disable it for tests. But yes, that > > > Repository: > rC Clang > > https://reviews.llvm.org/D44883 > > > > ___ > 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] D45405: [clang-tidy] [modernize-use-auto] Add a threshold for minimal type name length to be replaced with 'auto'
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. Looks good. Thank you! https://reviews.llvm.org/D45405 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45414: [clang-tidy] add missing assignment operations in hicpp-signed-bitwise
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LG aside from a diagnostic wording nit.
Comment at: clang-tidy/hicpp/SignedBitwiseCheck.cpp:92
+ if (N.getNodeAs("std_type"))
+diag(Location, "shifting a value of the standardized bitmask types");
+ else
How about: "shifting a value of bitmask type"
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45414
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43578: -ftime-report switch support in Clang
rsmith added a comment. In https://reviews.llvm.org/D43578#1062984, @thakis wrote: > Also, please add cfe-commits to clang changes. Since this wasn't here and the > patch wasn't seen by clang folks, since > ftime-report-template-decl.cppwarnings is still failing on the bots (e.g. > http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16388) > and since it breaks the shared bot, maybe we should revert for now and > reland when the issues are addressed? Yes, please. This was not appropriately reviewed, breaks bots, and appears to be wrong/inappropriate in multiple ways. Repository: rL LLVM https://reviews.llvm.org/D43578 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
arichardson added a comment. I'm also often restricted to using printf for debugging so this looks really useful! However, before committing this I feel like the test should also verify that the format strings that are generated are sensible. Also what should happens when you have enum members in your struct or maybe even C++ pointers to members? Comment at: test/Sema/builtin-dump-struct.c:42 + __builtin_dump_struct(&a, goodfunc2); +} I think there should also be a test here that we get an error when the struct contains bitfields instead of crashing/generating nonsense in CodeGen. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang-tools-extra] r329452 - [clang-tidy] Fix compilation for ParentVirtualCallCheck.cpp
On Mon, Apr 9, 2018 at 8:09 PM Zinovy Nis wrote:
> I had compilation errors here on MVSV@PSP and for ARM:
>
>
>
> FAILED: /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE
> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
> -Itools/clang/tools/extra/clang-tidy/bugprone
> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone
>
> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/include
> -Itools/clang/include -Iinclude
> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include
> -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W
> -Wno-unused-parameter -Wwrite-strings -Wcast-qual
> -Wno-missing-field-initializers -pedantic -Wno-long-long
> -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment
> -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual
> -fno-strict-aliasing -O3-UNDEBUG -fno-exceptions -fno-rtti -MMD -MT
> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o
> -MF
> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o.d
> -o
> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o
> -c
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
> In function 'bool clang::tidy::bugprone::isParentOf(const
> clang::CXXRecordDecl&, const clang::CXXRecordDecl&)':
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:30:63:
> error: use of 'auto' in lambda parameter declaration only available with
> -std=c++14 or -std=gnu++14
>const auto ClassIter = llvm::find_if(ThisClass.bases(), [=](auto &Base) {
>^
>
> But that seems to be unrelated to llvm::find_if? The compiler is
complaining about the use of `auto` in the lambda argument type.
>
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
> In lambda function:
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:31:27:
> error: request for member 'getType' in 'Base', which is of non-class type
> 'int'
> auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();
>^
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
> In function 'std::__cxx11::string
> clang::tidy::bugprone::getExprAsString(const clang::Expr&,
> clang::ASTContext&)':
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:77:48:
> error: no matching function for call to 'remove_if(std::__cxx11::string&,
> )'
>Text.erase(llvm::remove_if(Text, std::isspace), Text.end());
> ^
>
> It also doesn't look specific to llvm::remove_if. That can be fixed either
by wrapping std::isspace into a lambda or by using a static_cast to select the right overload.
>
> In file included from
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/StringRef.h:13:0,
> from
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/StringMap.h:17,
> from
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidyOptions.h:14,
> from
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidyDiagnosticConsumer.h:13,
> from
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidy.h:13,
> from
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.h:13,
> from
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:10:
> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/STLExtras.h:886:6:
> note: candidate: template decltype
> (llvm::adl_begin(Range)) llvm::remove_if(R&&, UnaryPredicate)
> auto remove_if(R &&Range, UnaryPredicate P) -> decltype(adl_begin(Range)) {
>
Re: [clang-tools-extra] r329452 - [clang-tidy] Fix compilation for ParentVirtualCallCheck.cpp
Looks you are right, but I was in hurry trying to fix build bot failures
ASAP.
вт, 10 апр. 2018 г. в 18:28, Alexander Kornienko :
> On Mon, Apr 9, 2018 at 8:09 PM Zinovy Nis wrote:
>
>> I had compilation errors here on MVSV@PSP and for ARM:
>>
>>
>>
>> FAILED: /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE
>> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>> -Itools/clang/tools/extra/clang-tidy/bugprone
>> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone
>>
>> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/include
>> -Itools/clang/include -Iinclude
>> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include
>> -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W
>> -Wno-unused-parameter -Wwrite-strings -Wcast-qual
>> -Wno-missing-field-initializers -pedantic -Wno-long-long
>> -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment
>> -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual
>> -fno-strict-aliasing -O3-UNDEBUG -fno-exceptions -fno-rtti -MMD -MT
>> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o
>> -MF
>> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o.d
>> -o
>> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o
>> -c
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
>> In function 'bool clang::tidy::bugprone::isParentOf(const
>> clang::CXXRecordDecl&, const clang::CXXRecordDecl&)':
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:30:63:
>> error: use of 'auto' in lambda parameter declaration only available with
>> -std=c++14 or -std=gnu++14
>>const auto ClassIter = llvm::find_if(ThisClass.bases(), [=](auto &Base) {
>>^
>>
>> But that seems to be unrelated to llvm::find_if? The compiler is
> complaining about the use of `auto` in the lambda argument type.
>
>
>>
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
>> In lambda function:
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:31:27:
>> error: request for member 'getType' in 'Base', which is of non-class type
>> 'int'
>> auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();
>>^
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
>> In function 'std::__cxx11::string
>> clang::tidy::bugprone::getExprAsString(const clang::Expr&,
>> clang::ASTContext&)':
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:77:48:
>> error: no matching function for call to 'remove_if(std::__cxx11::string&,
>> )'
>>Text.erase(llvm::remove_if(Text, std::isspace), Text.end());
>> ^
>>
>> It also doesn't look specific to llvm::remove_if. That can be fixed
> either by wrapping std::isspace into a lambda or by using a static_cast (*)(int)> to select the right overload.
>
>
>>
>> In file included from
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/StringRef.h:13:0,
>> from
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/StringMap.h:17,
>> from
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidyOptions.h:14,
>> from
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidyDiagnosticConsumer.h:13,
>> from
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidy.h:13,
>> from
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.h:13,
>> from
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:10:
>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/l
[PATCH] D45491: [analyzer] Do not invalidate the `this` pointer.
MTC created this revision.
MTC added reviewers: NoQ, george.karpenkov, a.sidorin, seaneveson, szepet.
Herald added subscribers: cfe-commits, rnkovacs, xazax.hun.
MTC edited the summary of this revision.
`this` pointer is not an l-value, although we have modeled `CXXThisRegion` for
`this` pointer, we can only bind it once, which is when we start to inline
method. And this patch fixes https://bugs.llvm.org/show_bug.cgi?id=35506.
In addition, I didn't find any other cases other than loop-widen that could
invalidate `this` pointer.
Repository:
rC Clang
https://reviews.llvm.org/D45491
Files:
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/this-pointer.cpp
Index: test/Analysis/this-pointer.cpp
===
--- /dev/null
+++ test/Analysis/this-pointer.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config widen-loops=true -analyzer-disable-retry-exhausted -verify %s
+
+void clang_analyzer_eval(bool);
+
+// 'this' pointer is not an lvalue, we should not invalidate it.
+namespace this_pointer_after_loop_widen {
+class A {
+public:
+ A() {
+int count = 10;
+do {
+} while (count--);
+ }
+};
+
+void goo(A a);
+void func1() {
+ goo(A()); // no-crash
+}
+
+struct B {
+ int mem;
+ B() : mem(0) {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 0;
+ }
+};
+
+void func2() {
+ B b;
+ clang_analyzer_eval(b.mem == 0); // expected-warning{{TRUE}}
+}
+
+struct C {
+ int mem;
+ C() : mem(0) {}
+ void set() {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 10;
+ }
+};
+
+void func3() {
+ C c;
+ clang_analyzer_eval(c.mem == 0); // expected-warning{{TRUE}}
+ c.set();
+ clang_analyzer_eval(c.mem == 10); // expected-warning{{TRUE}}
+}
+
+struct D {
+ int mem;
+ D() : mem(0) {}
+ void set() {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 10;
+ }
+};
+
+void func4() {
+ D *d = new D;
+ clang_analyzer_eval(d->mem == 0); // expected-warning{{TRUE}}
+ d->set();
+ clang_analyzer_eval(d->mem == 10); // expected-warning{{TRUE}}
+}
+
+struct E {
+ int mem;
+ E() : mem(0) {}
+ void set() {
+for (int i = 0; i < 10; ++i) {
+}
+setAux();
+ }
+ void setAux() {
+this->mem = 10;
+ }
+};
+
+void func6() {
+ E e;
+ e.set();
+ clang_analyzer_eval(e.mem == 10); // expected-warning{{TRUE}}
+}
+} // namespace this_pointer_after_loop_widen
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1021,6 +1021,10 @@
void invalidateRegionsWorker::VisitCluster(const MemRegion *baseR,
const ClusterBindings *C) {
+ // 'this' pointer is not an lvalue, we should not invalidate it.
+ if (CXXThisRegion::classof(baseR))
+return;
+
bool PreserveRegionsContents =
ITraits.hasTrait(baseR,
RegionAndSymbolInvalidationTraits::TK_PreserveContents);
@@ -2050,8 +2054,12 @@
R = GetElementZeroRegion(SR, T);
}
+ assert((!CXXThisRegion::classof(R) ||
+ CXXThisRegion::classof(R) && !B.lookup(R)) &&
+ "'this' pointer is not an l-value and is not assignable");
+
// Clear out bindings that may overlap with this binding.
- RegionBindingsRef NewB = removeSubRegionBindings(B, cast(R));
+ RegionBindingsRef NewB = removeSubRegionBindings(B, cast(R));
return NewB.addBinding(BindingKey::Make(R, BindingKey::Direct), V);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'
alexfh added a comment. In https://reviews.llvm.org/D45392#1061960, @Wizard wrote: > In https://reviews.llvm.org/D45392#1061433, @alexfh wrote: > > > I wonder whether the readability-identifier-naming check could be extended > > to support this use case instead of adding a new check specifically for > > underscores in ivar names? > > > Hmm readability-identifier-naming is a C++ check but this one is only for > ObjC. I prefer putting them in separate places unless they work for both > languages. I see no reasons why this check can't work for ObjC, if the handling of ObjC-specific AST nodes is added to it. > Moreover, readability-identifier-naming always runs all matchers and apply > the same checks on all identifiers. It has some sort of a hierarchical structure of rules that allow it to only touch a certain subset of identifiers. E.g. configure different naming styles for local variables and local constants. What it's lacking is a good documentation for all of these options =\ > We have to change at least part of the structure to make it applicable for > this check. Yes, the existing check may need some modifications to do what this check needs to do, but I'd expect these modifications to be quite small, and we would potentially get a much more useful and generic tool instead of "one check per type of named entity per naming style" situation. > And readability-identifier-naming is not in google default clang-tidy check > list yet. We will even need more work to import it. IIUC, it can be configured to only verify certain kinds of named entities. Thus there's no requirement to make it work with every aspect of our naming rules. > So I think creating a new simple ObjC-specific check is a better way here. I'll respectfully disagree here. I would prefer to have a generic solution, if it's feasible. And so far, it looks like it is. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45392 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. LG. Thanks! https://reviews.llvm.org/D45059 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang-tools-extra] r329452 - [clang-tidy] Fix compilation for ParentVirtualCallCheck.cpp
I totally get it, no worries. But if you have free cycles, it would be nice
to brush this code up again.
On Tue, Apr 10, 2018 at 5:32 PM Zinovy Nis wrote:
> Looks you are right, but I was in hurry trying to fix build bot failures
> ASAP.
>
> вт, 10 апр. 2018 г. в 18:28, Alexander Kornienko :
>
>> On Mon, Apr 9, 2018 at 8:09 PM Zinovy Nis wrote:
>>
>>> I had compilation errors here on MVSV@PSP and for ARM:
>>>
>>>
>>>
>>> FAILED: /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE
>>> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>>> -Itools/clang/tools/extra/clang-tidy/bugprone
>>> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone
>>>
>>> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/include
>>> -Itools/clang/include -Iinclude
>>> -I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include
>>> -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W
>>> -Wno-unused-parameter -Wwrite-strings -Wcast-qual
>>> -Wno-missing-field-initializers -pedantic -Wno-long-long
>>> -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment
>>> -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual
>>> -fno-strict-aliasing -O3-UNDEBUG -fno-exceptions -fno-rtti -MMD -MT
>>> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o
>>> -MF
>>> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o.d
>>> -o
>>> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o
>>> -c
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
>>> In function 'bool clang::tidy::bugprone::isParentOf(const
>>> clang::CXXRecordDecl&, const clang::CXXRecordDecl&)':
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:30:63:
>>> error: use of 'auto' in lambda parameter declaration only available with
>>> -std=c++14 or -std=gnu++14
>>>const auto ClassIter = llvm::find_if(ThisClass.bases(), [=](auto &Base) {
>>>^
>>>
>>> But that seems to be unrelated to llvm::find_if? The compiler is
>> complaining about the use of `auto` in the lambda argument type.
>>
>>
>>>
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
>>> In lambda function:
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:31:27:
>>> error: request for member 'getType' in 'Base', which is of non-class type
>>> 'int'
>>> auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();
>>>^
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
>>> In function 'std::__cxx11::string
>>> clang::tidy::bugprone::getExprAsString(const clang::Expr&,
>>> clang::ASTContext&)':
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:77:48:
>>> error: no matching function for call to 'remove_if(std::__cxx11::string&,
>>> )'
>>>Text.erase(llvm::remove_if(Text, std::isspace), Text.end());
>>> ^
>>>
>>> It also doesn't look specific to llvm::remove_if. That can be fixed
>> either by wrapping std::isspace into a lambda or by using a static_cast> (*)(int)> to select the right overload.
>>
>>
>>>
>>> In file included from
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/StringRef.h:13:0,
>>> from
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/StringMap.h:17,
>>> from
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidyOptions.h:14,
>>> from
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidyDiagnosticConsumer.h:13,
>>> from
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidy.h:13,
>>> from
>>> /home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck
[PATCH] D45212: [HIP] Let CUDA toolchain support HIP language mode and amdgpu
yaxunl updated this revision to Diff 141863.
yaxunl marked 2 inline comments as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Separate file type changes to another patch.
https://reviews.llvm.org/D45212
Files:
include/clang/Driver/Options.td
include/clang/Driver/ToolChain.h
lib/Driver/Driver.cpp
lib/Driver/ToolChain.cpp
lib/Driver/ToolChains/Cuda.cpp
lib/Driver/ToolChains/Cuda.h
test/Driver/cuda-phases.cu
Index: test/Driver/cuda-phases.cu
===
--- test/Driver/cuda-phases.cu
+++ test/Driver/cuda-phases.cu
@@ -7,24 +7,29 @@
// REQUIRES: clang-driver
// REQUIRES: powerpc-registered-target
// REQUIRES: nvptx-registered-target
-
+// REQUIRES: amdgpu-registered-target
//
// Test single gpu architecture with complete compilation.
//
// RUN: %clang -target powerpc64le-ibm-linux-gnu -ccc-print-phases --cuda-gpu-arch=sm_30 %s 2>&1 \
-// RUN: | FileCheck -check-prefix=BIN %s
-// BIN-DAG: [[P0:[0-9]+]]: input, "{{.*}}cuda-phases.cu", cuda, (host-cuda)
-// BIN-DAG: [[P1:[0-9]+]]: preprocessor, {[[P0]]}, cuda-cpp-output, (host-cuda)
+// RUN: | FileCheck -check-prefixes=BIN,BIN_NV %s
+// RUN: %clang -x hip -target powerpc64le-ibm-linux-gnu -ccc-print-phases --cuda-gpu-arch=gfx803 %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=BIN,BIN_AMD %s
+// BIN_NV-DAG: [[P0:[0-9]+]]: input, "{{.*}}cuda-phases.cu", [[T:cuda]], (host-cuda)
+// BIN_AMD-DAG: [[P0:[0-9]+]]: input, "{{.*}}cuda-phases.cu", [[T:hip]], (host-cuda)
+// BIN-DAG: [[P1:[0-9]+]]: preprocessor, {[[P0]]}, [[T]]-cpp-output, (host-cuda)
// BIN-DAG: [[P2:[0-9]+]]: compiler, {[[P1]]}, ir, (host-cuda)
-// BIN-DAG: [[P3:[0-9]+]]: input, "{{.*}}cuda-phases.cu", cuda, (device-cuda, sm_30)
-// BIN-DAG: [[P4:[0-9]+]]: preprocessor, {[[P3]]}, cuda-cpp-output, (device-cuda, sm_30)
-// BIN-DAG: [[P5:[0-9]+]]: compiler, {[[P4]]}, ir, (device-cuda, sm_30)
-// BIN-DAG: [[P6:[0-9]+]]: backend, {[[P5]]}, assembler, (device-cuda, sm_30)
-// BIN-DAG: [[P7:[0-9]+]]: assembler, {[[P6]]}, object, (device-cuda, sm_30)
-// BIN-DAG: [[P8:[0-9]+]]: offload, "device-cuda (nvptx64-nvidia-cuda:sm_30)" {[[P7]]}, object
-// BIN-DAG: [[P9:[0-9]+]]: offload, "device-cuda (nvptx64-nvidia-cuda:sm_30)" {[[P6]]}, assembler
+// BIN_NV-DAG: [[P3:[0-9]+]]: input, "{{.*}}cuda-phases.cu", [[T]], (device-cuda, [[ARCH:sm_30]])
+// BIN_AMD-DAG: [[P3:[0-9]+]]: input, "{{.*}}cuda-phases.cu", [[T]], (device-cuda, [[ARCH:gfx803]])
+// BIN-DAG: [[P4:[0-9]+]]: preprocessor, {[[P3]]}, [[T]]-cpp-output, (device-cuda, [[ARCH]])
+// BIN-DAG: [[P5:[0-9]+]]: compiler, {[[P4]]}, ir, (device-cuda, [[ARCH]])
+// BIN-DAG: [[P6:[0-9]+]]: backend, {[[P5]]}, assembler, (device-cuda, [[ARCH]])
+// BIN-DAG: [[P7:[0-9]+]]: assembler, {[[P6]]}, object, (device-cuda, [[ARCH]])
+// BIN_NV-DAG: [[P8:[0-9]+]]: offload, "device-cuda ([[TRIPLE:nvptx64-nvidia-cuda]]:[[ARCH]])" {[[P7]]}, object
+// BIN_AMD-DAG: [[P8:[0-9]+]]: offload, "device-cuda ([[TRIPLE:amdgcn-amd-amdhsa]]:[[ARCH]])" {[[P7]]}, object
+// BIN-DAG: [[P9:[0-9]+]]: offload, "device-cuda ([[TRIPLE]]:[[ARCH]])" {[[P6]]}, assembler
// BIN-DAG: [[P10:[0-9]+]]: linker, {[[P8]], [[P9]]}, cuda-fatbin, (device-cuda)
-// BIN-DAG: [[P11:[0-9]+]]: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {[[P2]]}, "device-cuda (nvptx64-nvidia-cuda)" {[[P10]]}, ir
+// BIN-DAG: [[P11:[0-9]+]]: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {[[P2]]}, "device-cuda ([[TRIPLE]])" {[[P10]]}, ir
// BIN-DAG: [[P12:[0-9]+]]: backend, {[[P11]]}, assembler, (host-cuda)
// BIN-DAG: [[P13:[0-9]+]]: assembler, {[[P12]]}, object, (host-cuda)
// BIN-DAG: [[P14:[0-9]+]]: linker, {[[P13]]}, image, (host-cuda)
@@ -34,40 +39,44 @@
//
// RUN: %clang -target powerpc64le-ibm-linux-gnu -ccc-print-phases --cuda-gpu-arch=sm_30 %s -S 2>&1 \
// RUN: | FileCheck -check-prefix=ASM %s
-// ASM-DAG: [[P0:[0-9]+]]: input, "{{.*}}cuda-phases.cu", cuda, (device-cuda, sm_30)
-// ASM-DAG: [[P1:[0-9]+]]: preprocessor, {[[P0]]}, cuda-cpp-output, (device-cuda, sm_30)
-// ASM-DAG: [[P2:[0-9]+]]: compiler, {[[P1]]}, ir, (device-cuda, sm_30)
-// ASM-DAG: [[P3:[0-9]+]]: backend, {[[P2]]}, assembler, (device-cuda, sm_30)
-// ASM-DAG: [[P4:[0-9]+]]: offload, "device-cuda (nvptx64-nvidia-cuda:sm_30)" {[[P3]]}, assembler
-// ASM-DAG: [[P5:[0-9]+]]: input, "{{.*}}cuda-phases.cu", cuda, (host-cuda)
-// ASM-DAG: [[P6:[0-9]+]]: preprocessor, {[[P5]]}, cuda-cpp-output, (host-cuda)
+// RUN: %clang -x hip -target powerpc64le-ibm-linux-gnu -ccc-print-phases --cuda-gpu-arch=gfx803 %s -S 2>&1 \
+// RUN: | FileCheck -check-prefix=ASM %s
+// ASM-DAG: [[P0:[0-9]+]]: input, "{{.*}}cuda-phases.cu", [[T:cuda|hip]], (device-cuda, [[ARCH:sm_30|gfx803]])
+// ASM-DAG: [[P1:[0-9]+]]: preprocessor, {[[P0]]}, [[T]]-cpp-output, (device-cuda, [[ARCH]])
+// ASM-DAG: [[P2:[0-9]+]]: compiler, {[[P1]]}, ir, (device-cuda, [[ARCH]])
+// ASM-DAG: [[P3:[0-9]+]]: backend, {[[P2]]}, assembler, (device-cuda, [[ARCH]])
+// ASM-DAG: [[P4:[0-9]+]]: offload,
r329714 - I removed the failed test.
Author: avt77
Date: Tue Apr 10 08:45:43 2018
New Revision: 329714
URL: http://llvm.org/viewvc/llvm-project?rev=329714&view=rev
Log:
I removed the failed test.
Removed:
cfe/trunk/test/Frontend/ftime-report-template-decl.cpp
Removed: cfe/trunk/test/Frontend/ftime-report-template-decl.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/ftime-report-template-decl.cpp?rev=329713&view=auto
==
--- cfe/trunk/test/Frontend/ftime-report-template-decl.cpp (original)
+++ cfe/trunk/test/Frontend/ftime-report-template-decl.cpp (removed)
@@ -1,147 +0,0 @@
-// RUN: %clang %s -S -o - -ftime-report 2>&1 | FileCheck %s
-// RUN: %clang %s -S -o - -fdelayed-template-parsing
-DDELAYED_TEMPLATE_PARSING -ftime-report 2>&1 | FileCheck %s
-
-// Template function declarations
-template
-void foo();
-template
-void foo();
-
-// Template function definitions.
-template
-void foo() {}
-
-// Template class (forward) declarations
-template
-struct A;
-template
-struct b;
-template
-struct C;
-template
-struct D;
-
-// Forward declarations with default parameters?
-template
-class X1;
-template
-class X2;
-
-// Forward declarations w/template template parameters
-template class T>
-class TTP1;
-template class>
-class TTP2;
-template class T>
-class TTP5;
-
-// Forward declarations with non-type params
-template
-class NTP0;
-template
-class NTP1;
-template
-class NTP2;
-template
-class NTP3;
-template
-class NTP4;
-template
-class NTP5;
-template
-class NTP6;
-template
-class NTP7;
-
-// Template class declarations
-template
-struct A {};
-template
-struct B {};
-
-namespace PR6184 {
-namespace N {
-template
-void bar(typename T::x);
-}
-
-template
-void N::bar(typename T::x) {}
-}
-
-// This PR occurred only in template parsing mode.
-namespace PR17637 {
-template
-struct L {
- template
- struct O {
-template
-static void Fun(U);
- };
-};
-
-template
-template
-template
-void L::O::Fun(U) {}
-
-void Instantiate() { L<0>::O::Fun(0); }
-}
-
-namespace explicit_partial_specializations {
-typedef char (&oneT)[1];
-typedef char (&twoT)[2];
-typedef char (&threeT)[3];
-typedef char (&fourT)[4];
-typedef char (&fiveT)[5];
-typedef char (&sixT)[6];
-
-char one[1];
-char two[2];
-char three[3];
-char four[4];
-char five[5];
-char six[6];
-
-template
-struct bool_ { typedef int type; };
-template <>
-struct bool_ {};
-
-#define XCAT(x, y) x##y
-#define CAT(x, y) XCAT(x, y)
-#define sassert(_b_) bool_<(_b_)>::type CAT(var, __LINE__);
-
-template
-struct L {
- template
- struct O {
-template
-static oneT Fun(U);
- };
-};
-template
-template
-template
-oneT L::O::Fun(U) { return one; }
-
-template <>
-template <>
-template
-oneT L<0>::O::Fun(U) { return one; }
-
-void Instantiate() {
- sassert(sizeof(L<0>::O::Fun(0)) == sizeof(one));
- sassert(sizeof(L<0>::O::Fun(0)) == sizeof(one));
-}
-}
-
-// CHECK: = Clang Parser =
-// CHECK: ---User Time---
-// CHECK: Parse Top Level Decl
-// CHECK: Parse Template
-// CHECK: Parse Function Definition
-// CHECK: PP Append Macro
-// CHECK: Scope manipulation
-// CHECK: PP Find Handler
-// CHECK: Total
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45454: Make __gcov_flush visible outside a shared library
davidxl added a comment. With this change, gcov_flush will resolve to the copy in the main executable for each shared library -- this is not the desired the behavior. https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45212: [HIP] Let CUDA toolchain support HIP language mode and amdgpu
yaxunl added a comment. In https://reviews.llvm.org/D45212#1060628, @Hahnfeld wrote: > Can this revision be split further? The summary mentions many things that > might make up multiple independent changes... I separated file type changes to https://reviews.llvm.org/D45489 and deferred some other changes for future. It is kind of difficult to split this patch further since they depend on each other. https://reviews.llvm.org/D45212 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang-tools-extra] r329452 - [clang-tidy] Fix compilation for ParentVirtualCallCheck.cpp
OK)
вт, 10 апр. 2018 г. в 18:44, Alexander Kornienko :
> I totally get it, no worries. But if you have free cycles, it would be
> nice to brush this code up again.
>
>
> On Tue, Apr 10, 2018 at 5:32 PM Zinovy Nis wrote:
>
>> Looks you are right, but I was in hurry trying to fix build bot failures
>> ASAP.
>>
>> вт, 10 апр. 2018 г. в 18:28, Alexander Kornienko :
>>
>>> On Mon, Apr 9, 2018 at 8:09 PM Zinovy Nis wrote:
>>>
I had compilation errors here on MVSV@PSP and for ARM:
FAILED: /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-Itools/clang/tools/extra/clang-tidy/bugprone
-I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone
-I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/include
-Itools/clang/include -Iinclude
-I/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include
-fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W
-Wno-unused-parameter -Wwrite-strings -Wcast-qual
-Wno-missing-field-initializers -pedantic -Wno-long-long
-Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment
-ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual
-fno-strict-aliasing -O3-UNDEBUG -fno-exceptions -fno-rtti -MMD -MT
tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o
-MF
tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o.d
-o
tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/clangTidyBugproneModule.dir/ParentVirtualCallCheck.cpp.o
-c
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
In function 'bool clang::tidy::bugprone::isParentOf(const
clang::CXXRecordDecl&, const clang::CXXRecordDecl&)':
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:30:63:
error: use of 'auto' in lambda parameter declaration only available with
-std=c++14 or -std=gnu++14
const auto ClassIter = llvm::find_if(ThisClass.bases(), [=](auto &Base)
{
^
But that seems to be unrelated to llvm::find_if? The compiler is
>>> complaining about the use of `auto` in the lambda argument type.
>>>
>>>
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
In lambda function:
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:31:27:
error: request for member 'getType' in 'Base', which is of non-class type
'int'
auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();
^
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:
In function 'std::__cxx11::string
clang::tidy::bugprone::getExprAsString(const clang::Expr&,
clang::ASTContext&)':
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:77:48:
error: no matching function for call to 'remove_if(std::__cxx11::string&,
)'
Text.erase(llvm::remove_if(Text, std::isspace), Text.end());
^
It also doesn't look specific to llvm::remove_if. That can be fixed
>>> either by wrapping std::isspace into a lambda or by using a static_cast>> (*)(int)> to select the right overload.
>>>
>>>
In file included from
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/StringRef.h:13:0,
from
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/include/llvm/ADT/StringMap.h:17,
from
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidyOptions.h:14,
from
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidyDiagnosticConsumer.h:13,
from
/home/uweigand/sandbox/buildbot/clang-s390x-linux-multistage/llvm/tools/clang/tools/extra/clang-tidy/bugprone/../ClangTidy.h:13,
Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
If you have a concrete suggestion of how to suppress this warning for user-defined operators just in unit tests, great. I don’t think 8 easily-suppressed warnings is an unacceptably large false-positive problem, though. Most warnings have similar problems, and the standard cannot possibly be “must never fire on code where the programmer is actually happy with the behavior”. John. On Tue, Apr 10, 2018 at 17:12 Nico Weber wrote: > "False positive" means "warning fires but didn't find anything > interesting", not "warning fires while being technically correct". So all > these instances do count as false positives. > > clang tries super hard to make sure that every time a warning fires, it is > useful for a dev to fix it. If you build with warnings enabled, that should > be a rewarding experience. Often, this means dialing back a warning to not > warn in cases where it would make sense in theory when in practice the > warning doesn't find much compared to the amount of noise it generates. > This is why for example clang's -Woverloaded-virtual is usable while gcc's > isn't (or wasn't last I checked a while ago) – gcc fires always when it's > technically correct to do so, clang only when it actually matters in > practice. > > On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via > cfe-commits wrote: > >> lebedev.ri added a comment. >> >> In https://reviews.llvm.org/D44883#1063003, @thakis wrote: >> >> > This landing made our clang trunk bots do an evaluation of this warning >> :-P It fired 8 times, all false positives, and all from unit tests testing >> that operator= works for self-assignment. ( >> https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has >> the exact details) It looks like the same issue exists in LLVM itself too, >> https://reviews.llvm.org/D45082 >> >> >> Right, i guess i only built the chrome binary itself, not the tests, good >> to know... >> >> > Now tests often need warning suppressions for things like this, and >> this in itself doesn't seem horrible. However, this change takes a warning >> that was previously 100% noise-free in practice and makes it pretty noisy – >> without a big benefit in practice. I get that it's beneficial in theory, >> but that's true of many warnings. >> > >> > Based on how this warning does in practice, I think it might be better >> for the static analyzer, which has a lower bar for false positives. >> >> Noisy in the sense that it correctly diagnoses a self-assignment where >> one **intended** to have self-assignment. >> And unsurprisingly, it happened in the unit-tests, as was expected ^ in >> previous comments. >> **So far** there are no truly false-positives noise (at least no reports >> of it). >> >> We could help workaround that the way i initially suggested, by keeping >> this new part of the diag under it's own sub-flag, >> and suggest to disable it for tests. But yes, that >> > >> >> Repository: >> rC Clang >> >> https://reviews.llvm.org/D44883 >> >> >> >> ___ >> 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] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/FunctionSizeCheck.cpp:31
+!(isa(VD) || isa(VD)) &&
+!VD->getLocation().isMacroID())
+ Info.Variables++;
This isn't the restriction I was envisioning. I was thinking more along the
lines of:
```
bool VisitStmtExpr(StmtExpr *SE) override {
++StructNesting;
Base::VisitStmtExpr(SE);
--StructNesting;
return true;
}
```
Basically -- treat a statement expression the same as an inner struct or lambda
because the variables declared within it are scoped *only* to the statement
expression.
You could argue that we should also add: `if (!SE->getLocation.isMacroID()) {
Base::VisitStmtExpr(SE); }` to the top of the function so that you only treat
statement expressions that are themselves in a macro expansion get this special
treatment. e.g.,
```
void one_var() {
(void)({int a = 12; a;});
}
#define M ({int a = 12; a;})
void zero_var() {
(void)M;
}
```
I don't have strong opinions on this, but weakly lean towards treating all
statement expressions the same way.
Comment at: docs/clang-tidy/checks/readability-function-size.rst:44-46
+ Please do note that function parameters are not counted here.
+ Also, the variables declared in macros expansion, and in nested lambdas,
+ nested classes are not counted.
I'd combine these into a serial list, like: `Please note that function
parameters and variables declared in macro expansions, lambdas, and nested
class inline functions are not counted.`
Comment at: test/clang-tidy/readability-function-size.cpp:207-212
+void variables_8() {
+ int a, b;
+ struct A {
+A(int c, int d);
+ };
+}
JonasToth wrote:
> lebedev.ri wrote:
> > aaron.ballman wrote:
> > > lebedev.ri wrote:
> > > > aaron.ballman wrote:
> > > > > JonasToth wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > lebedev.ri wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > lebedev.ri wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > lebedev.ri wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > I think the current behavior here is correct and
> > > > > > > > > > > > > > the previous behavior was incorrect. However, it
> > > > > > > > > > > > > > brings up an interesting question about what to do
> > > > > > > > > > > > > > here:
> > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > void f() {
> > > > > > > > > > > > > > struct S {
> > > > > > > > > > > > > > void bar() {
> > > > > > > > > > > > > > int a, b;
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > > };
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > Does `f()` contain zero variables or two? I would
> > > > > > > > > > > > > > contend that it has no variables because S::bar()
> > > > > > > > > > > > > > is a different scope than f(). But I can see a case
> > > > > > > > > > > > > > being made about the complexity of f() being
> > > > > > > > > > > > > > increased by the presence of the local class
> > > > > > > > > > > > > > definition. Perhaps this is a different facet of
> > > > > > > > > > > > > > the test about number of types?
> > > > > > > > > > > > > As previously briefly discussed in IRC, i
> > > > > > > > > > > > > **strongly** believe that the current behavior is
> > > > > > > > > > > > > correct, and `readability-function-size`
> > > > > > > > > > > > > should analyze/diagnose the function as a whole,
> > > > > > > > > > > > > including all sub-classes/sub-functions.
> > > > > > > > > > > > Do you know of any coding standards related to this
> > > > > > > > > > > > check that weigh in on this?
> > > > > > > > > > > >
> > > > > > > > > > > > What do you think about this:
> > > > > > > > > > > > ```
> > > > > > > > > > > > #define SWAP(x, y) ({__typeof__(x) temp = x; x = y; y =
> > > > > > > > > > > > x;})
> > > > > > > > > > > >
> > > > > > > > > > > > void f() {
> > > > > > > > > > > > int a = 10, b = 12;
> > > > > > > > > > > > SWAP(a, b);
> > > > > > > > > > > > }
> > > > > > > > > > > > ```
> > > > > > > > > > > > Does f() have two variables or three? Should presence
> > > > > > > > > > > > of the `SWAP` macro cause this code to be more complex
> > > > > > > > > > > > due to having too many variables?
> > > > > > > > > > > Datapoint: the doc
> > > > > > > > > > > (`docs/clang-tidy/checks/readability-function-size.rst`)
> > > > > > > > > > > actually already states that macros *are* counted.
> > > > > > > > > > >
> > > > > > > > > > > ```
> > > > > > > > > > > .. option:: StatementThreshold
> > > > > > > > > > >
> > > > > > > > > > >Flag functions exceeding this number of statements.
> > > > > > > > > > > This may differ
> > > > > > > > > > >significantly from the number of lines for macro-heavy
> > > > > > > > > > > c
Re: r329714 - I removed the failed test.
This is not an appropriate response to a test failing. Please revert the
entire patch, then put it up for review again and this time request review
from cfe-commits.
On Tue, 10 Apr 2018, 16:48 Andrew V. Tischenko via cfe-commits, <
cfe-commits@lists.llvm.org> wrote:
> Author: avt77
> Date: Tue Apr 10 08:45:43 2018
> New Revision: 329714
>
> URL: http://llvm.org/viewvc/llvm-project?rev=329714&view=rev
> Log:
> I removed the failed test.
>
> Removed:
> cfe/trunk/test/Frontend/ftime-report-template-decl.cpp
>
> Removed: cfe/trunk/test/Frontend/ftime-report-template-decl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/ftime-report-template-decl.cpp?rev=329713&view=auto
>
> ==
> --- cfe/trunk/test/Frontend/ftime-report-template-decl.cpp (original)
> +++ cfe/trunk/test/Frontend/ftime-report-template-decl.cpp (removed)
> @@ -1,147 +0,0 @@
> -// RUN: %clang %s -S -o - -ftime-report 2>&1 | FileCheck %s
> -// RUN: %clang %s -S -o - -fdelayed-template-parsing
> -DDELAYED_TEMPLATE_PARSING -ftime-report 2>&1 | FileCheck %s
> -
> -// Template function declarations
> -template
> -void foo();
> -template
> -void foo();
> -
> -// Template function definitions.
> -template
> -void foo() {}
> -
> -// Template class (forward) declarations
> -template
> -struct A;
> -template
> -struct b;
> -template
> -struct C;
> -template
> -struct D;
> -
> -// Forward declarations with default parameters?
> -template
> -class X1;
> -template
> -class X2;
> -
> -// Forward declarations w/template template parameters
> -template class T>
> -class TTP1;
> -template class>
> -class TTP2;
> -template class T>
> -class TTP5;
> -
> -// Forward declarations with non-type params
> -template
> -class NTP0;
> -template
> -class NTP1;
> -template
> -class NTP2;
> -template
> -class NTP3;
> -template
> -class NTP4;
> -template
> -class NTP5;
> -template
> -class NTP6;
> -template
> -class NTP7;
> -
> -// Template class declarations
> -template
> -struct A {};
> -template
> -struct B {};
> -
> -namespace PR6184 {
> -namespace N {
> -template
> -void bar(typename T::x);
> -}
> -
> -template
> -void N::bar(typename T::x) {}
> -}
> -
> -// This PR occurred only in template parsing mode.
> -namespace PR17637 {
> -template
> -struct L {
> - template
> - struct O {
> -template
> -static void Fun(U);
> - };
> -};
> -
> -template
> -template
> -template
> -void L::O::Fun(U) {}
> -
> -void Instantiate() { L<0>::O::Fun(0); }
> -}
> -
> -namespace explicit_partial_specializations {
> -typedef char (&oneT)[1];
> -typedef char (&twoT)[2];
> -typedef char (&threeT)[3];
> -typedef char (&fourT)[4];
> -typedef char (&fiveT)[5];
> -typedef char (&sixT)[6];
> -
> -char one[1];
> -char two[2];
> -char three[3];
> -char four[4];
> -char five[5];
> -char six[6];
> -
> -template
> -struct bool_ { typedef int type; };
> -template <>
> -struct bool_ {};
> -
> -#define XCAT(x, y) x##y
> -#define CAT(x, y) XCAT(x, y)
> -#define sassert(_b_) bool_<(_b_)>::type CAT(var, __LINE__);
> -
> -template
> -struct L {
> - template
> - struct O {
> -template
> -static oneT Fun(U);
> - };
> -};
> -template
> -template
> -template
> -oneT L::O::Fun(U) { return one; }
> -
> -template <>
> -template <>
> -template
> -oneT L<0>::O::Fun(U) { return one; }
> -
> -void Instantiate() {
> - sassert(sizeof(L<0>::O::Fun(0)) == sizeof(one));
> - sassert(sizeof(L<0>::O::Fun(0)) == sizeof(one));
> -}
> -}
> -
> -// CHECK: = Clang Parser =
> -// CHECK: ---User Time---
> -// CHECK: Parse Top Level Decl
> -// CHECK: Parse Template
> -// CHECK: Parse Function Definition
> -// CHECK: PP Append Macro
> -// CHECK: Scope manipulation
> -// CHECK: PP Find Handler
> -// CHECK: Total
>
>
> ___
> 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] D44988: [Sema] Fix decrement availability for built-in types
jkorous-apple updated this revision to Diff 141865.
jkorous-apple added a comment.
Added test for decrement being disabled for bool.
Fixed comment in test (will put into separate NFC commit).
https://reviews.llvm.org/D44988
Files:
Sema/SemaOverload.cpp
SemaCXX/overloaded-builtin-operators.cpp
Index: SemaCXX/overloaded-builtin-operators.cpp
===
--- SemaCXX/overloaded-builtin-operators.cpp
+++ SemaCXX/overloaded-builtin-operators.cpp
@@ -62,6 +62,10 @@
// FIXME: should pass (void)static_cast(islong(e1 % e2));
}
+struct BoolRef {
+ operator bool&();
+};
+
struct ShortRef { // expected-note{{candidate function (the implicit copy
assignment operator) not viable}}
#if __cplusplus >= 201103L // C++11 or later
// expected-note@-2 {{candidate function (the implicit move assignment
operator) not viable}}
@@ -73,6 +77,10 @@
operator volatile long&();
};
+struct FloatRef {
+ operator float&();
+};
+
struct XpmfRef { // expected-note{{candidate function (the implicit copy
assignment operator) not viable}}
#if __cplusplus >= 201103L // C++11 or later
// expected-note@-2 {{candidate function (the implicit move assignment
operator) not viable}}
@@ -84,13 +92,19 @@
operator E2&();
};
-void g(ShortRef sr, LongRef lr, E2Ref e2_ref, XpmfRef pmf_ref) {
+void g(BoolRef br, ShortRef sr, LongRef lr, FloatRef fr, E2Ref e2_ref, XpmfRef
pmf_ref) {
// C++ [over.built]p3
short s1 = sr++;
- // C++ [over.built]p3
+ // C++ [over.built]p4
long l1 = lr--;
+ // C++ [over.built]p4
+ float f1 = fr--;
+
+ // C++ [over.built]p4
+ bool b2 = br--; // expected-error{{cannot decrement value of type 'BoolRef'}}
+
// C++ [over.built]p18
short& sr1 = (sr *= lr);
volatile long& lr1 = (lr *= sr);
Index: Sema/SemaOverload.cpp
===
--- Sema/SemaOverload.cpp
+++ Sema/SemaOverload.cpp
@@ -7796,10 +7796,12 @@
if (!HasArithmeticOrEnumeralCandidateType)
return;
-for (unsigned Arith = (Op == OO_PlusPlus? 0 : 1);
- Arith < NumArithmeticTypes; ++Arith) {
+for (unsigned Arith = 0; Arith < NumArithmeticTypes; ++Arith) {
+ const auto TypeOfT = ArithmeticTypes[Arith];
+ if (Op == OO_MinusMinus && TypeOfT == S.Context.BoolTy)
+continue;
addPlusPlusMinusMinusStyleOverloads(
-ArithmeticTypes[Arith],
+TypeOfT,
VisibleTypeConversionsQuals.hasVolatile(),
VisibleTypeConversionsQuals.hasRestrict());
}
Index: SemaCXX/overloaded-builtin-operators.cpp
===
--- SemaCXX/overloaded-builtin-operators.cpp
+++ SemaCXX/overloaded-builtin-operators.cpp
@@ -62,6 +62,10 @@
// FIXME: should pass (void)static_cast(islong(e1 % e2));
}
+struct BoolRef {
+ operator bool&();
+};
+
struct ShortRef { // expected-note{{candidate function (the implicit copy assignment operator) not viable}}
#if __cplusplus >= 201103L // C++11 or later
// expected-note@-2 {{candidate function (the implicit move assignment operator) not viable}}
@@ -73,6 +77,10 @@
operator volatile long&();
};
+struct FloatRef {
+ operator float&();
+};
+
struct XpmfRef { // expected-note{{candidate function (the implicit copy assignment operator) not viable}}
#if __cplusplus >= 201103L // C++11 or later
// expected-note@-2 {{candidate function (the implicit move assignment operator) not viable}}
@@ -84,13 +92,19 @@
operator E2&();
};
-void g(ShortRef sr, LongRef lr, E2Ref e2_ref, XpmfRef pmf_ref) {
+void g(BoolRef br, ShortRef sr, LongRef lr, FloatRef fr, E2Ref e2_ref, XpmfRef pmf_ref) {
// C++ [over.built]p3
short s1 = sr++;
- // C++ [over.built]p3
+ // C++ [over.built]p4
long l1 = lr--;
+ // C++ [over.built]p4
+ float f1 = fr--;
+
+ // C++ [over.built]p4
+ bool b2 = br--; // expected-error{{cannot decrement value of type 'BoolRef'}}
+
// C++ [over.built]p18
short& sr1 = (sr *= lr);
volatile long& lr1 = (lr *= sr);
Index: Sema/SemaOverload.cpp
===
--- Sema/SemaOverload.cpp
+++ Sema/SemaOverload.cpp
@@ -7796,10 +7796,12 @@
if (!HasArithmeticOrEnumeralCandidateType)
return;
-for (unsigned Arith = (Op == OO_PlusPlus? 0 : 1);
- Arith < NumArithmeticTypes; ++Arith) {
+for (unsigned Arith = 0; Arith < NumArithmeticTypes; ++Arith) {
+ const auto TypeOfT = ArithmeticTypes[Arith];
+ if (Op == OO_MinusMinus && TypeOfT == S.Context.BoolTy)
+continue;
addPlusPlusMinusMinusStyleOverloads(
-ArithmeticTypes[Arith],
+TypeOfT,
VisibleTypeConversionsQuals.hasVolatile(),
VisibleTypeConversionsQuals.hasRestrict());
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44988: [Sema] Fix decrement availability for built-in types
jkorous-apple added a comment. Spot on with the negative test idea! Should've done that myself. Thanks. https://reviews.llvm.org/D44988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
It's more the fact that that's /all/ the warning improvement has found so far. If it was 8 false positives & also found 80 actionable bugs/bad code, that'd be one thing. Now, admittedly, maybe it would help find bugs that people usually catch with a unit test, etc but never make it to checked in code - that's always harder to evaluate though Google has some infrastructure for it at least. On Tue, Apr 10, 2018 at 9:07 AM John McCall wrote: > If you have a concrete suggestion of how to suppress this warning for > user-defined operators just in unit tests, great. I don’t think 8 > easily-suppressed warnings is an unacceptably large false-positive problem, > though. Most warnings have similar problems, and the standard cannot > possibly be “must never fire on code where the programmer is actually happy > with the behavior”. > > John. > On Tue, Apr 10, 2018 at 17:12 Nico Weber wrote: > >> "False positive" means "warning fires but didn't find anything >> interesting", not "warning fires while being technically correct". So all >> these instances do count as false positives. >> >> clang tries super hard to make sure that every time a warning fires, it >> is useful for a dev to fix it. If you build with warnings enabled, that >> should be a rewarding experience. Often, this means dialing back a warning >> to not warn in cases where it would make sense in theory when in practice >> the warning doesn't find much compared to the amount of noise it generates. >> This is why for example clang's -Woverloaded-virtual is usable while gcc's >> isn't (or wasn't last I checked a while ago) – gcc fires always when it's >> technically correct to do so, clang only when it actually matters in >> practice. >> >> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via >> cfe-commits wrote: >> >>> lebedev.ri added a comment. >>> >>> In https://reviews.llvm.org/D44883#1063003, @thakis wrote: >>> >>> > This landing made our clang trunk bots do an evaluation of this >>> warning :-P It fired 8 times, all false positives, and all from unit tests >>> testing that operator= works for self-assignment. ( >>> https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has >>> the exact details) It looks like the same issue exists in LLVM itself too, >>> https://reviews.llvm.org/D45082 >>> >>> >>> Right, i guess i only built the chrome binary itself, not the tests, >>> good to know... >>> >>> > Now tests often need warning suppressions for things like this, and >>> this in itself doesn't seem horrible. However, this change takes a warning >>> that was previously 100% noise-free in practice and makes it pretty noisy – >>> without a big benefit in practice. I get that it's beneficial in theory, >>> but that's true of many warnings. >>> > >>> > Based on how this warning does in practice, I think it might be better >>> for the static analyzer, which has a lower bar for false positives. >>> >>> Noisy in the sense that it correctly diagnoses a self-assignment where >>> one **intended** to have self-assignment. >>> And unsurprisingly, it happened in the unit-tests, as was expected ^ in >>> previous comments. >>> **So far** there are no truly false-positives noise (at least no reports >>> of it). >>> >>> We could help workaround that the way i initially suggested, by keeping >>> this new part of the diag under it's own sub-flag, >>> and suggest to disable it for tests. But yes, that >>> >> >>> >>> Repository: >>> rC Clang >>> >>> https://reviews.llvm.org/D44883 >>> >>> >>> >>> ___ >>> 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: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
Because *so far* it has been running on the existing code, which i guess was already pretty warning free, and was run through the static analyzer, which obviously should catch such issues. Do you always use [clang] static analyzer, each time you build? Great! It is indeed very good to do so. But does everyone? This particular issue is easily detectable without full static analysis, and as it is seen from the differential, was very straight-forward to implement as a simple extension of pre-existing -Wself-assign. So if this is really truly unacceptable false-positive rate, ok, sure, file a RC bug, and i will split it so that it can be simply disabled for *tests*. That being said, i understand the reasons behind "keep clang diagnostics false-positive free". I don't really want to change that. On Tue, Apr 10, 2018 at 7:13 PM, David Blaikie wrote: > It's more the fact that that's /all/ the warning improvement has found so > far. If it was 8 false positives & also found 80 actionable bugs/bad code, > that'd be one thing. > > Now, admittedly, maybe it would help find bugs that people usually catch > with a unit test, etc but never make it to checked in code - that's always > harder to evaluate though Google has some infrastructure for it at least. > > On Tue, Apr 10, 2018 at 9:07 AM John McCall wrote: >> >> If you have a concrete suggestion of how to suppress this warning for >> user-defined operators just in unit tests, great. I don’t think 8 >> easily-suppressed warnings is an unacceptably large false-positive problem, >> though. Most warnings have similar problems, and the standard cannot >> possibly be “must never fire on code where the programmer is actually happy >> with the behavior”. >> >> John. >> On Tue, Apr 10, 2018 at 17:12 Nico Weber wrote: >>> >>> "False positive" means "warning fires but didn't find anything >>> interesting", not "warning fires while being technically correct". So all >>> these instances do count as false positives. >>> >>> clang tries super hard to make sure that every time a warning fires, it >>> is useful for a dev to fix it. If you build with warnings enabled, that >>> should be a rewarding experience. Often, this means dialing back a warning >>> to not warn in cases where it would make sense in theory when in practice >>> the warning doesn't find much compared to the amount of noise it generates. >>> This is why for example clang's -Woverloaded-virtual is usable while gcc's >>> isn't (or wasn't last I checked a while ago) – gcc fires always when it's >>> technically correct to do so, clang only when it actually matters in >>> practice. >>> >>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via >>> cfe-commits wrote: lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1063003, @thakis wrote: > This landing made our clang trunk bots do an evaluation of this > warning :-P It fired 8 times, all false positives, and all from unit > tests > testing that operator= works for self-assignment. > (https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has > the > exact details) It looks like the same issue exists in LLVM itself too, > https://reviews.llvm.org/D45082 Right, i guess i only built the chrome binary itself, not the tests, good to know... > Now tests often need warning suppressions for things like this, and > this in itself doesn't seem horrible. However, this change takes a > warning > that was previously 100% noise-free in practice and makes it pretty > noisy – > without a big benefit in practice. I get that it's beneficial in theory, > but > that's true of many warnings. > > Based on how this warning does in practice, I think it might be better > for the static analyzer, which has a lower bar for false positives. Noisy in the sense that it correctly diagnoses a self-assignment where one **intended** to have self-assignment. And unsurprisingly, it happened in the unit-tests, as was expected ^ in previous comments. **So far** there are no truly false-positives noise (at least no reports of it). We could help workaround that the way i initially suggested, by keeping this new part of the diag under it's own sub-flag, and suggest to disable it for tests. But yes, that Repository: rC Clang https://reviews.llvm.org/D44883 ___ 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] D45212: [HIP] Let CUDA toolchain support HIP language mode and amdgpu
Hahnfeld added a comment. In https://reviews.llvm.org/D45212#1063186, @yaxunl wrote: > In https://reviews.llvm.org/D45212#1060628, @Hahnfeld wrote: > > > Can this revision be split further? The summary mentions many things that > > might make up multiple independent changes... > > > I separated file type changes to https://reviews.llvm.org/D45489 and deferred > some other changes for future. > > It is kind of difficult to split this patch further since they depend on each > other. You can add `Related Revisions` to make this easy to see. https://reviews.llvm.org/D45212 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev wrote: > Because *so far* it has been running on the existing code, which i guess > was already pretty warning free, and was run through the static analyzer, which obviously should catch such issues. > Existing code this has been run over isn't necessarily run against the static analyzer already. (this has been run against a subset of (maybe all of, I forget) google, which isn't static analyzer clean by any stretch (I'm not even sure if the LLVM codebase itself is static analyzer clean)). > Do you always use [clang] static analyzer, each time you build? > Great! It is indeed very good to do so. But does everyone? > > This particular issue is easily detectable without full static analysis, > and as it is seen from the differential, was very straight-forward to > implement > as a simple extension of pre-existing -Wself-assign. > > So if this is really truly unacceptable false-positive rate, ok, sure, > file a RC bug, > and i will split it so that it can be simply disabled for *tests*. > > That being said, i understand the reasons behind "keep clang diagnostics > false-positive free". I don't really want to change that. > > On Tue, Apr 10, 2018 at 7:13 PM, David Blaikie wrote: > > It's more the fact that that's /all/ the warning improvement has found so > > far. If it was 8 false positives & also found 80 actionable bugs/bad > code, > > that'd be one thing. > > > > Now, admittedly, maybe it would help find bugs that people usually catch > > with a unit test, etc but never make it to checked in code - that's > always > > harder to evaluate though Google has some infrastructure for it at least. > > > > On Tue, Apr 10, 2018 at 9:07 AM John McCall wrote: > >> > >> If you have a concrete suggestion of how to suppress this warning for > >> user-defined operators just in unit tests, great. I don’t think 8 > >> easily-suppressed warnings is an unacceptably large false-positive > problem, > >> though. Most warnings have similar problems, and the standard cannot > >> possibly be “must never fire on code where the programmer is actually > happy > >> with the behavior”. > >> > >> John. > >> On Tue, Apr 10, 2018 at 17:12 Nico Weber wrote: > >>> > >>> "False positive" means "warning fires but didn't find anything > >>> interesting", not "warning fires while being technically correct". So > all > >>> these instances do count as false positives. > >>> > >>> clang tries super hard to make sure that every time a warning fires, it > >>> is useful for a dev to fix it. If you build with warnings enabled, that > >>> should be a rewarding experience. Often, this means dialing back a > warning > >>> to not warn in cases where it would make sense in theory when in > practice > >>> the warning doesn't find much compared to the amount of noise it > generates. > >>> This is why for example clang's -Woverloaded-virtual is usable while > gcc's > >>> isn't (or wasn't last I checked a while ago) – gcc fires always when > it's > >>> technically correct to do so, clang only when it actually matters in > >>> practice. > >>> > >>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via > >>> cfe-commits wrote: > > lebedev.ri added a comment. > > In https://reviews.llvm.org/D44883#1063003, @thakis wrote: > > > This landing made our clang trunk bots do an evaluation of this > > warning :-P It fired 8 times, all false positives, and all from > unit tests > > testing that operator= works for self-assignment. > > (https://chromium-review.googlesource.com/c/chromium/src/+/1000856 > has the > > exact details) It looks like the same issue exists in LLVM itself > too, > > https://reviews.llvm.org/D45082 > > > Right, i guess i only built the chrome binary itself, not the tests, > good to know... > > > Now tests often need warning suppressions for things like this, and > > this in itself doesn't seem horrible. However, this change takes a > warning > > that was previously 100% noise-free in practice and makes it pretty > noisy – > > without a big benefit in practice. I get that it's beneficial in > theory, but > > that's true of many warnings. > > > > Based on how this warning does in practice, I think it might be > better > > for the static analyzer, which has a lower bar for false positives. > > Noisy in the sense that it correctly diagnoses a self-assignment where > one **intended** to have self-assignment. > And unsurprisingly, it happened in the unit-tests, as was expected ^ > in > previous comments. > **So far** there are no truly false-positives noise (at least no > reports > of it). > > We could help workaround that the way i initially suggested, by > keeping > this new part of the diag under it's own sub-flag, > and suggest to disable it for tests. But yes, that > > > > Repository: >
Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
On Tue, Apr 10, 2018 at 12:13 PM, David Blaikie wrote: > It's more the fact that that's /all/ the warning improvement has found so > far. If it was 8 false positives & also found 80 actionable bugs/bad code, > that'd be one thing. > Right. We used to hold ourselves to very high standards for warnings. I'd like us to keep doing that. Now, admittedly, maybe it would help find bugs that people usually catch > with a unit test, etc but never make it to checked in code - that's always > harder to evaluate though Google has some infrastructure for it at least. > > On Tue, Apr 10, 2018 at 9:07 AM John McCall wrote: > >> If you have a concrete suggestion of how to suppress this warning for >> user-defined operators just in unit tests, great. I don’t think 8 >> easily-suppressed warnings is an unacceptably large false-positive problem, >> though. Most warnings have similar problems, and the standard cannot >> possibly be “must never fire on code where the programmer is actually happy >> with the behavior”. >> >> John. >> On Tue, Apr 10, 2018 at 17:12 Nico Weber wrote: >> >>> "False positive" means "warning fires but didn't find anything >>> interesting", not "warning fires while being technically correct". So all >>> these instances do count as false positives. >>> >>> clang tries super hard to make sure that every time a warning fires, it >>> is useful for a dev to fix it. If you build with warnings enabled, that >>> should be a rewarding experience. Often, this means dialing back a warning >>> to not warn in cases where it would make sense in theory when in practice >>> the warning doesn't find much compared to the amount of noise it generates. >>> This is why for example clang's -Woverloaded-virtual is usable while gcc's >>> isn't (or wasn't last I checked a while ago) – gcc fires always when it's >>> technically correct to do so, clang only when it actually matters in >>> practice. >>> >>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via >>> cfe-commits wrote: >>> lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1063003, @thakis wrote: > This landing made our clang trunk bots do an evaluation of this warning :-P It fired 8 times, all false positives, and all from unit tests testing that operator= works for self-assignment. ( https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has the exact details) It looks like the same issue exists in LLVM itself too, https://reviews.llvm.org/D45082 Right, i guess i only built the chrome binary itself, not the tests, good to know... > Now tests often need warning suppressions for things like this, and this in itself doesn't seem horrible. However, this change takes a warning that was previously 100% noise-free in practice and makes it pretty noisy – without a big benefit in practice. I get that it's beneficial in theory, but that's true of many warnings. > > Based on how this warning does in practice, I think it might be better for the static analyzer, which has a lower bar for false positives. Noisy in the sense that it correctly diagnoses a self-assignment where one **intended** to have self-assignment. And unsurprisingly, it happened in the unit-tests, as was expected ^ in previous comments. **So far** there are no truly false-positives noise (at least no reports of it). We could help workaround that the way i initially suggested, by keeping this new part of the diag under it's own sub-flag, and suggest to disable it for tests. But yes, that >>> Repository: rC Clang https://reviews.llvm.org/D44883 ___ 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: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
Yeah, -Wself-assign in general is about catching a class of live-coding errors which is pretty unlikely to ever make it into committed code, since the code would need to be dead/redundant for the mistake to not be noticed. So it’s specifically a rather poor match for the static analyzer, and I would not expect it to catch much of anything on production code. John. On Tue, Apr 10, 2018 at 18:27 David Blaikie wrote: > On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev > wrote: > >> Because *so far* it has been running on the existing code, which i guess >> was already pretty warning free, and was run through the static analyzer, > > which obviously should catch such issues. >> > > Existing code this has been run over isn't necessarily run against the > static analyzer already. (this has been run against a subset of (maybe all > of, I forget) google, which isn't static analyzer clean by any stretch (I'm > not even sure if the LLVM codebase itself is static analyzer clean)). > > >> Do you always use [clang] static analyzer, each time you build? >> Great! It is indeed very good to do so. But does everyone? >> >> This particular issue is easily detectable without full static analysis, >> and as it is seen from the differential, was very straight-forward to >> implement >> as a simple extension of pre-existing -Wself-assign. >> >> So if this is really truly unacceptable false-positive rate, ok, sure, >> file a RC bug, >> and i will split it so that it can be simply disabled for *tests*. >> >> That being said, i understand the reasons behind "keep clang diagnostics >> false-positive free". I don't really want to change that. >> >> On Tue, Apr 10, 2018 at 7:13 PM, David Blaikie >> wrote: >> > It's more the fact that that's /all/ the warning improvement has found >> so >> > far. If it was 8 false positives & also found 80 actionable bugs/bad >> code, >> > that'd be one thing. >> > >> > Now, admittedly, maybe it would help find bugs that people usually catch >> > with a unit test, etc but never make it to checked in code - that's >> always >> > harder to evaluate though Google has some infrastructure for it at >> least. >> > >> > On Tue, Apr 10, 2018 at 9:07 AM John McCall wrote: >> >> >> >> If you have a concrete suggestion of how to suppress this warning for >> >> user-defined operators just in unit tests, great. I don’t think 8 >> >> easily-suppressed warnings is an unacceptably large false-positive >> problem, >> >> though. Most warnings have similar problems, and the standard cannot >> >> possibly be “must never fire on code where the programmer is actually >> happy >> >> with the behavior”. >> >> >> >> John. >> >> On Tue, Apr 10, 2018 at 17:12 Nico Weber wrote: >> >>> >> >>> "False positive" means "warning fires but didn't find anything >> >>> interesting", not "warning fires while being technically correct". So >> all >> >>> these instances do count as false positives. >> >>> >> >>> clang tries super hard to make sure that every time a warning fires, >> it >> >>> is useful for a dev to fix it. If you build with warnings enabled, >> that >> >>> should be a rewarding experience. Often, this means dialing back a >> warning >> >>> to not warn in cases where it would make sense in theory when in >> practice >> >>> the warning doesn't find much compared to the amount of noise it >> generates. >> >>> This is why for example clang's -Woverloaded-virtual is usable while >> gcc's >> >>> isn't (or wasn't last I checked a while ago) – gcc fires always when >> it's >> >>> technically correct to do so, clang only when it actually matters in >> >>> practice. >> >>> >> >>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via >> >>> cfe-commits wrote: >> >> lebedev.ri added a comment. >> >> In https://reviews.llvm.org/D44883#1063003, @thakis wrote: >> >> > This landing made our clang trunk bots do an evaluation of this >> > warning :-P It fired 8 times, all false positives, and all from >> unit tests >> > testing that operator= works for self-assignment. >> > (https://chromium-review.googlesource.com/c/chromium/src/+/1000856 >> has the >> > exact details) It looks like the same issue exists in LLVM itself >> too, >> > https://reviews.llvm.org/D45082 >> >> >> Right, i guess i only built the chrome binary itself, not the tests, >> good to know... >> >> > Now tests often need warning suppressions for things like this, and >> > this in itself doesn't seem horrible. However, this change takes a >> warning >> > that was previously 100% noise-free in practice and makes it >> pretty noisy – >> > without a big benefit in practice. I get that it's beneficial in >> theory, but >> > that's true of many warnings. >> > >> > Based on how this warning does in practice, I think it might be >> better >> > for the static analyzer, which has a lower bar for false positives. >> >> Noisy in the sense th
[PATCH] D45414: [clang-tidy] add missing assignment operations in hicpp-signed-bitwise
JonasToth added inline comments.
Comment at: clang-tidy/hicpp/SignedBitwiseCheck.cpp:92
+ if (N.getNodeAs("std_type"))
+diag(Location, "shifting a value of the standardized bitmask types");
+ else
aaron.ballman wrote:
> How about: "shifting a value of bitmask type"
Not sure about that.
The general bitmasks are covered by the second `diag`.
This one should only trigger if a shift with the standardized bitmask types
occurs. This exception is necessary because those are allowed for the other
bitwise operations(&, |, ^), but i decided that shifting them makes no sense.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45414
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)
Any good ideas on how to evaluate it, then? (in addition to/other than something like Google where we can track the diagnostic for - a few months?) On Tue, Apr 10, 2018 at 9:34 AM John McCall wrote: > Yeah, -Wself-assign in general is about catching a class of live-coding > errors which is pretty unlikely to ever make it into committed code, since > the code would need to be dead/redundant for the mistake to not be noticed. > So it’s specifically a rather poor match for the static analyzer, and I > would not expect it to catch much of anything on production code. > > John. > On Tue, Apr 10, 2018 at 18:27 David Blaikie wrote: > >> On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev >> wrote: >> >>> Because *so far* it has been running on the existing code, which i guess >>> was already pretty warning free, and was run through the static >>> analyzer, >> >> which obviously should catch such issues. >>> >> >> Existing code this has been run over isn't necessarily run against the >> static analyzer already. (this has been run against a subset of (maybe all >> of, I forget) google, which isn't static analyzer clean by any stretch (I'm >> not even sure if the LLVM codebase itself is static analyzer clean)). >> >> >>> Do you always use [clang] static analyzer, each time you build? >>> Great! It is indeed very good to do so. But does everyone? >>> >>> This particular issue is easily detectable without full static analysis, >>> and as it is seen from the differential, was very straight-forward to >>> implement >>> as a simple extension of pre-existing -Wself-assign. >>> >>> So if this is really truly unacceptable false-positive rate, ok, sure, >>> file a RC bug, >>> and i will split it so that it can be simply disabled for *tests*. >>> >>> That being said, i understand the reasons behind "keep clang diagnostics >>> false-positive free". I don't really want to change that. >>> >>> On Tue, Apr 10, 2018 at 7:13 PM, David Blaikie >>> wrote: >>> > It's more the fact that that's /all/ the warning improvement has found >>> so >>> > far. If it was 8 false positives & also found 80 actionable bugs/bad >>> code, >>> > that'd be one thing. >>> > >>> > Now, admittedly, maybe it would help find bugs that people usually >>> catch >>> > with a unit test, etc but never make it to checked in code - that's >>> always >>> > harder to evaluate though Google has some infrastructure for it at >>> least. >>> > >>> > On Tue, Apr 10, 2018 at 9:07 AM John McCall >>> wrote: >>> >> >>> >> If you have a concrete suggestion of how to suppress this warning for >>> >> user-defined operators just in unit tests, great. I don’t think 8 >>> >> easily-suppressed warnings is an unacceptably large false-positive >>> problem, >>> >> though. Most warnings have similar problems, and the standard cannot >>> >> possibly be “must never fire on code where the programmer is actually >>> happy >>> >> with the behavior”. >>> >> >>> >> John. >>> >> On Tue, Apr 10, 2018 at 17:12 Nico Weber wrote: >>> >>> >>> >>> "False positive" means "warning fires but didn't find anything >>> >>> interesting", not "warning fires while being technically correct". >>> So all >>> >>> these instances do count as false positives. >>> >>> >>> >>> clang tries super hard to make sure that every time a warning fires, >>> it >>> >>> is useful for a dev to fix it. If you build with warnings enabled, >>> that >>> >>> should be a rewarding experience. Often, this means dialing back a >>> warning >>> >>> to not warn in cases where it would make sense in theory when in >>> practice >>> >>> the warning doesn't find much compared to the amount of noise it >>> generates. >>> >>> This is why for example clang's -Woverloaded-virtual is usable while >>> gcc's >>> >>> isn't (or wasn't last I checked a while ago) – gcc fires always when >>> it's >>> >>> technically correct to do so, clang only when it actually matters in >>> >>> practice. >>> >>> >>> >>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via >>> >>> cfe-commits wrote: >>> >>> lebedev.ri added a comment. >>> >>> In https://reviews.llvm.org/D44883#1063003, @thakis wrote: >>> >>> > This landing made our clang trunk bots do an evaluation of this >>> > warning :-P It fired 8 times, all false positives, and all from >>> unit tests >>> > testing that operator= works for self-assignment. >>> > ( >>> https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has >>> the >>> > exact details) It looks like the same issue exists in LLVM itself >>> too, >>> > https://reviews.llvm.org/D45082 >>> >>> >>> Right, i guess i only built the chrome binary itself, not the tests, >>> good to know... >>> >>> > Now tests often need warning suppressions for things like this, >>> and >>> > this in itself doesn't seem horrible. However, this change takes >>> a warning >>> > that was previously 100% noise-free in practice and makes it >>> pretty no
[PATCH] D45417: [analyzer] Don't crash on printing ConcreteInt of size >64 bits
a.sidorin updated this revision to Diff 141869.
a.sidorin edited the summary of this revision.
a.sidorin added a comment.
After thinking a bit more, I have removed the FIXME at all: dumping SVal is an
extremely rare event so this shouldn't affect the performance.
Repository:
rC Clang
https://reviews.llvm.org/D45417
Files:
lib/StaticAnalyzer/Core/SVals.cpp
test/Analysis/egraph-dump-int128.c
Index: test/Analysis/egraph-dump-int128.c
===
--- /dev/null
+++ test/Analysis/egraph-dump-int128.c
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection %s -verify
+
+void clang_analyzer_dump(unsigned __int128 x);
+
+void testDumpInt128() {
+ clang_analyzer_dump((unsigned __int128)5 << 64); //
expected-warning{{92233720368547758080 U128b}}
+}
Index: lib/StaticAnalyzer/Core/SVals.cpp
===
--- lib/StaticAnalyzer/Core/SVals.cpp
+++ lib/StaticAnalyzer/Core/SVals.cpp
@@ -300,13 +300,9 @@
void NonLoc::dumpToStream(raw_ostream &os) const {
switch (getSubKind()) {
case nonloc::ConcreteIntKind: {
- const nonloc::ConcreteInt& C = castAs();
- if (C.getValue().isUnsigned())
-os << C.getValue().getZExtValue();
- else
-os << C.getValue().getSExtValue();
- os << ' ' << (C.getValue().isUnsigned() ? 'U' : 'S')
- << C.getValue().getBitWidth() << 'b';
+ const auto &Value = castAs().getValue();
+ os << Value << ' ' << (Value.isSigned() ? 'S' : 'U')
+ << Value.getBitWidth() << 'b';
break;
}
case nonloc::SymbolValKind:
Index: test/Analysis/egraph-dump-int128.c
===
--- /dev/null
+++ test/Analysis/egraph-dump-int128.c
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection %s -verify
+
+void clang_analyzer_dump(unsigned __int128 x);
+
+void testDumpInt128() {
+ clang_analyzer_dump((unsigned __int128)5 << 64); // expected-warning{{92233720368547758080 U128b}}
+}
Index: lib/StaticAnalyzer/Core/SVals.cpp
===
--- lib/StaticAnalyzer/Core/SVals.cpp
+++ lib/StaticAnalyzer/Core/SVals.cpp
@@ -300,13 +300,9 @@
void NonLoc::dumpToStream(raw_ostream &os) const {
switch (getSubKind()) {
case nonloc::ConcreteIntKind: {
- const nonloc::ConcreteInt& C = castAs();
- if (C.getValue().isUnsigned())
-os << C.getValue().getZExtValue();
- else
-os << C.getValue().getSExtValue();
- os << ' ' << (C.getValue().isUnsigned() ? 'U' : 'S')
- << C.getValue().getBitWidth() << 'b';
+ const auto &Value = castAs().getValue();
+ os << Value << ' ' << (Value.isSigned() ? 'S' : 'U')
+ << Value.getBitWidth() << 'b';
break;
}
case nonloc::SymbolValKind:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45417: [analyzer] Don't crash on printing ConcreteInt of size >64 bits
a.sidorin updated this revision to Diff 141870.
a.sidorin added a comment.
Renamed the test file.
Repository:
rC Clang
https://reviews.llvm.org/D45417
Files:
lib/StaticAnalyzer/Core/SVals.cpp
test/Analysis/sval-dump-int128.c
Index: test/Analysis/sval-dump-int128.c
===
--- /dev/null
+++ test/Analysis/sval-dump-int128.c
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection %s -verify
+
+void clang_analyzer_dump(unsigned __int128 x);
+
+void testDumpInt128() {
+ clang_analyzer_dump((unsigned __int128)5 << 64); //
expected-warning{{92233720368547758080 U128b}}
+}
Index: lib/StaticAnalyzer/Core/SVals.cpp
===
--- lib/StaticAnalyzer/Core/SVals.cpp
+++ lib/StaticAnalyzer/Core/SVals.cpp
@@ -300,13 +300,9 @@
void NonLoc::dumpToStream(raw_ostream &os) const {
switch (getSubKind()) {
case nonloc::ConcreteIntKind: {
- const nonloc::ConcreteInt& C = castAs();
- if (C.getValue().isUnsigned())
-os << C.getValue().getZExtValue();
- else
-os << C.getValue().getSExtValue();
- os << ' ' << (C.getValue().isUnsigned() ? 'U' : 'S')
- << C.getValue().getBitWidth() << 'b';
+ const auto &Value = castAs().getValue();
+ os << Value << ' ' << (Value.isSigned() ? 'S' : 'U')
+ << Value.getBitWidth() << 'b';
break;
}
case nonloc::SymbolValKind:
Index: test/Analysis/sval-dump-int128.c
===
--- /dev/null
+++ test/Analysis/sval-dump-int128.c
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection %s -verify
+
+void clang_analyzer_dump(unsigned __int128 x);
+
+void testDumpInt128() {
+ clang_analyzer_dump((unsigned __int128)5 << 64); // expected-warning{{92233720368547758080 U128b}}
+}
Index: lib/StaticAnalyzer/Core/SVals.cpp
===
--- lib/StaticAnalyzer/Core/SVals.cpp
+++ lib/StaticAnalyzer/Core/SVals.cpp
@@ -300,13 +300,9 @@
void NonLoc::dumpToStream(raw_ostream &os) const {
switch (getSubKind()) {
case nonloc::ConcreteIntKind: {
- const nonloc::ConcreteInt& C = castAs();
- if (C.getValue().isUnsigned())
-os << C.getValue().getZExtValue();
- else
-os << C.getValue().getSExtValue();
- os << ' ' << (C.getValue().isUnsigned() ? 'U' : 'S')
- << C.getValue().getBitWidth() << 'b';
+ const auto &Value = castAs().getValue();
+ os << Value << ' ' << (Value.isSigned() ? 'S' : 'U')
+ << Value.getBitWidth() << 'b';
break;
}
case nonloc::SymbolValKind:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
