r342667 - r342177 introduced a hint in cases where an #included file is not found. It tries to find a suggestion by removing leading or trailing non-alphanumeric characters and checking if a matching
Author: echristo
Date: Thu Sep 20 10:21:56 2018
New Revision: 342667
URL: http://llvm.org/viewvc/llvm-project?rev=342667=rev
Log:
r342177 introduced a hint in cases where an #included file is not found. It
tries to find a suggestion by removing leading or trailing non-alphanumeric
characters and checking if a matching file exists, then it reports an error
like:
include-likely-typo.c:3:10: error: '' file not found,
did you mean 'empty_file_to_include.h'?
^~~
"empty_file_to_include.h"
1 error generated.
However, if a hint is not found, the error message will show only the trimmed
name we use to look for a hint, so:
will result in:
include-leading-nonalpha-no-suggest.c:3:10: fatal error:
'non_existing_file_to_include.h' file not found
^~~~
1 error generated.
where the name reported after "fatal error:" doesn't match what the user wrote.
Patch by Jorge Gorbe!
Differential Revision: https://reviews.llvm.org/D52280
This change reports the original file name instead of the trimmed one when a
suggestion is not found.
Modified:
cfe/trunk/lib/Lex/PPDirectives.cpp
Modified: cfe/trunk/lib/Lex/PPDirectives.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPDirectives.cpp?rev=342667=342666=342667=diff
==
--- cfe/trunk/lib/Lex/PPDirectives.cpp (original)
+++ cfe/trunk/lib/Lex/PPDirectives.cpp Thu Sep 20 10:21:56 2018
@@ -1887,8 +1887,8 @@ void Preprocessor::HandleIncludeDirectiv
// Check for likely typos due to leading or trailing non-isAlphanumeric
// characters
+ StringRef OriginalFilename = Filename;
if (!File) {
-StringRef OriginalFilename = Filename;
while (!isAlphanumeric(Filename.front())) {
Filename = Filename.drop_front();
}
@@ -1915,7 +1915,7 @@ void Preprocessor::HandleIncludeDirectiv
// If the file is still not found, just go with the vanilla diagnostic
if (!File)
-Diag(FilenameTok, diag::err_pp_file_not_found) << Filename
+Diag(FilenameTok, diag::err_pp_file_not_found) << OriginalFilename
<< FilenameRange;
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52300: [clangd] Implement VByte PostingList compression
sammccall added a comment.
Very nice!
I think the data structures can be slightly tighter, and some of the
implementation could be easier to follow. But this seems like a nice win.
Right-sizing the vectors seems like an important optimization.
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:29
+ DecompressedChunk = ChunkIndex->decompress();
+ InnerIndex = begin(DecompressedChunk);
+}
nit: we generally use members (DecompressedChunk.begin()) unless actually
dealing with arrays or templates, since lookup rules are simpler
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:39
"Posting List iterator can't advance() at the end.");
-++Index;
+if (++InnerIndex != end(DecompressedChunk))
+ return; // Return if current chunk is not exhausted.
nit: I think this might be clearer with the special/unlikely cases (hit end)
inside the if:
```
if (++InnerIndex == DecompressedChunks.begin()) { // end of chunk
if (++ChunkIndex == Chunks.end()) // end of iterator
return;
DecompressedChunk = ChunkIndex->decompress();
InnerIndex = DecompressedChunk.begin();
}
```
also I think the indirection via `reachedEnd()` mostly serves to confuse here,
as the other lines deal with the data structures directly. It's not clear
(without reading the implementation) what the behavior is when class invariants
are violated.
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:58
+// does.
+if ((ChunkIndex != end(Chunks) - 1) && ((ChunkIndex + 1)->Head <= ID)) {
+ // Find the next chunk with Head >= ID.
this again puts the "normal case" (need to choose a chunk) inside the if(),
instead of the exceptional case.
In order to write this more naturally, I think pulling out a private helper
`advanceToChunk(DocID)` might be best here, you can early return from there.
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:61
+ ChunkIndex = std::lower_bound(
+ ChunkIndex, end(Chunks), ID,
+ [](const Chunk , const DocID ID) { return C.Head < ID; });
ChunkIndex + 1? You've already eliminated the current chunk.
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:62
+ ChunkIndex, end(Chunks), ID,
+ [](const Chunk , const DocID ID) { return C.Head < ID; });
+ if (reachedEnd())
This seems unneccesarily two-step (found the chunk... or it could be the first
element of the next).
Understandably, because std::*_bound has such a silly API.
You want to find the *last* chunk such that Head <= ID.
So find the first one with Head > ID, and subtract one.
std::lower_bound returns the first element for which its predicate is false.
Therefore:
```
ChunkIndex = std::lower_bound(ChunkIndex, Chunks.end(), ID,
[](const Chunk , const DocID D) { return C.Head <= ID; }) - 1;
```
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:63
+ [](const Chunk , const DocID ID) { return C.Head < ID; });
+ if (reachedEnd())
+return;
(again I'd avoid reachedEnd() here as you haven't reestablished invariants, so
it's easier to just deal with the data structures)
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:76
+// Return if the position was found in current chunk.
+if (InnerIndex != std::end(DecompressedChunk))
+ return;
(this can become an assert)
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:115
+ // Iterator over chunks.
+ std::vector::const_iterator ChunkIndex;
+ std::vector DecompressedChunk;
nit: please don't call these indexes if they're actually iterators:
CurrentChunk seems fine
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:116
+ std::vector::const_iterator ChunkIndex;
+ std::vector DecompressedChunk;
+ // Iterator over DecompressedChunk.
(again, SmallVector)
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:121
+/// Single-byte masks used for VByte compression bit manipulations.
+constexpr uint8_t SevenBytesMask = 0x7f; // 0b0111
move to the function where they're used
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:127
+/// Fills chunk with the maximum number of bits available.
+Chunk createChunk(DocID Head, std::queue ,
+ size_t DocumentsCount, size_t MeaningfulBytes) {
What's the purpose of this? Why can't the caller just construct the Chunk
themselves - what does the std::queue buy us?
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:140
+
+/// Byte offsets of Payload contents within DocID.
+const size_t Offsets[] = {0,
[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy
baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: alexfh, aaron.ballman, flx.
The three checks mentioned in the Title are two noisy if the code uses
intrusive smart (reference counting) pointers. LLVM/Clang is such a code, it
has lots of such types, e.g. StringRef, SymbolRef or ProgramStateRef, all of
them based llvm::IntrusiveRefCntPtr. Every time such a type is passed as
parameter, used for initialization or in a range-based for loop a false
positive warning is issued together with an (unnecessary) fix.
This patch changes the term "expensive to copy" to also regard the size of the
type so it disables warnings and fixes if the type is not greater than a
pointer. This removes false positives for intrusive smart pointers. False
positives for non-intrusive smart pointers are not addressed in this patch.
https://reviews.llvm.org/D52315
Files:
clang-tidy/utils/TypeTraits.cpp
docs/clang-tidy/checks/performance-for-range-copy.rst
docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
docs/clang-tidy/checks/performance-unnecessary-value-param.rst
test/clang-tidy/Inputs/performance-unnecessary-value-param/header-fixed.h
test/clang-tidy/Inputs/performance-unnecessary-value-param/header.h
test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp
test/clang-tidy/performance-for-range-copy.cpp
test/clang-tidy/performance-unnecessary-copy-initialization.cpp
test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -8,6 +8,9 @@
}
void nonConstMethod();
virtual ~ExpensiveToCopyType();
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
void mutate(ExpensiveToCopyType &);
@@ -27,6 +30,9 @@
iterator end();
const_iterator begin() const;
const_iterator end() const;
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
// This class simulates std::pair<>. It is trivially copy constructible
@@ -37,13 +43,19 @@
SomewhatTrivial(const SomewhatTrivial&) = default;
~SomewhatTrivial() = default;
SomewhatTrivial& operator=(const SomewhatTrivial&);
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
struct MoveOnlyType {
MoveOnlyType(const MoveOnlyType &) = delete;
MoveOnlyType(MoveOnlyType &&) = default;
~MoveOnlyType();
void constMethod() const;
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
struct ExpensiveMovableType {
@@ -53,6 +65,33 @@
ExpensiveMovableType =(const ExpensiveMovableType &) = default;
ExpensiveMovableType =(ExpensiveMovableType &&);
~ExpensiveMovableType();
+private:
+ // Greater than a pointer
+ long Data1, Data2;
+};
+
+// Intrusive smart pointers are usually passed by value
+template struct IntrusiveSmartPointerType {
+ IntrusiveSmartPointerType(const T* data);
+ IntrusiveSmartPointerType(const IntrusiveSmartPointerType& rhs);
+ const IntrusiveSmartPointerType& operator=(const T* data);
+ const IntrusiveSmartPointerType&
+ operator=(const IntrusiveSmartPointerType& data);
+
+ operator T*();
+private:
+ T *Data;
+};
+
+// Wrapper for builtin types used by intrusive smart pointers
+template
+class RefCntType {
+ unsigned count;
+ T value;
+public:
+ RefCntType(T val) : value(val) {}
+ operator T() { return value; }
+ friend class IntrusiveSmartPoitnerType;
};
void positiveExpensiveConstValue(const ExpensiveToCopyType Obj);
@@ -381,3 +420,10 @@
// CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) {
E.constReference();
}
+
+// Do not warn for intrusive smart pointers which are usually passed by value
+void negativeIntrusiveSmartPointer(const IntrusiveSmartPointerType> Obj);
+
+void negativeIntrusiveSmartPointer(const IntrusiveSmartPointerType> Obj) {
+}
+
Index: test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
+++ test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
@@ -6,6 +6,9 @@
}
void nonConstMethod();
virtual ~ExpensiveToCopyType();
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
void mutate(ExpensiveToCopyType &);
@@ -21,6 +24,9 @@
SomewhatTrivial(const SomewhatTrivial&) = default;
~SomewhatTrivial() = default;
SomewhatTrivial& operator=(const SomewhatTrivial&);
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
void positiveExpensiveConstValue(const ExpensiveToCopyType Obj);
Index: test/clang-tidy/performance-unnecessary-copy-initialization.cpp
===
---
r342666 - [OPENMP] Fix spelling of getLoopCounter (NFC)
Author: mikerice
Date: Thu Sep 20 10:19:41 2018
New Revision: 342666
URL: http://llvm.org/viewvc/llvm-project?rev=342666=rev
Log:
[OPENMP] Fix spelling of getLoopCounter (NFC)
Modified:
cfe/trunk/include/clang/AST/OpenMPClause.h
cfe/trunk/lib/AST/OpenMPClause.cpp
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/include/clang/AST/OpenMPClause.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OpenMPClause.h?rev=342666=342665=342666=diff
==
--- cfe/trunk/include/clang/AST/OpenMPClause.h (original)
+++ cfe/trunk/include/clang/AST/OpenMPClause.h Thu Sep 20 10:19:41 2018
@@ -990,8 +990,8 @@ public:
/// Set loop counter for the specified loop.
void setLoopCounter(unsigned NumLoop, Expr *Counter);
/// Get loops counter for the specified loop.
- Expr *getLoopCunter(unsigned NumLoop);
- const Expr *getLoopCunter(unsigned NumLoop) const;
+ Expr *getLoopCounter(unsigned NumLoop);
+ const Expr *getLoopCounter(unsigned NumLoop) const;
child_range children() { return child_range(, + 1);
}
Modified: cfe/trunk/lib/AST/OpenMPClause.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/OpenMPClause.cpp?rev=342666=342665=342666=diff
==
--- cfe/trunk/lib/AST/OpenMPClause.cpp (original)
+++ cfe/trunk/lib/AST/OpenMPClause.cpp Thu Sep 20 10:19:41 2018
@@ -222,12 +222,12 @@ void OMPOrderedClause::setLoopCounter(un
getTrailingObjects()[NumberOfLoops + NumLoop] = Counter;
}
-Expr *OMPOrderedClause::getLoopCunter(unsigned NumLoop) {
+Expr *OMPOrderedClause::getLoopCounter(unsigned NumLoop) {
assert(NumLoop < NumberOfLoops && "out of loops number.");
return getTrailingObjects()[NumberOfLoops + NumLoop];
}
-const Expr *OMPOrderedClause::getLoopCunter(unsigned NumLoop) const {
+const Expr *OMPOrderedClause::getLoopCounter(unsigned NumLoop) const {
assert(NumLoop < NumberOfLoops && "out of loops number.");
return getTrailingObjects()[NumberOfLoops + NumLoop];
}
Modified: cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp?rev=342666=342665=342666=diff
==
--- cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp Thu Sep 20 10:19:41 2018
@@ -1516,7 +1516,7 @@ void CodeGenFunction::EmitOMPPrivateLoop
for (unsigned I = S.getCollapsedNumber(),
E = C->getLoopNumIterations().size();
I < E; ++I) {
- const auto *DRE = cast(C->getLoopCunter(I));
+ const auto *DRE = cast(C->getLoopCounter(I));
const auto *VD = cast(DRE->getDecl());
// Override only those variables that are really emitted already.
if (LocalDeclMap.count(VD)) {
@@ -4966,7 +4966,7 @@ void CodeGenFunction::EmitSimpleOMPExecu
E = C->getLoopNumIterations().size();
I < E; ++I) {
if (const auto *VD = dyn_cast(
-cast(C->getLoopCunter(I))->getDecl())) {
+cast(C->getLoopCounter(I))->getDecl())) {
// Emit only those that were not explicitly referenced in
clauses.
if (!CGF.LocalDeclMap.count(VD))
CGF.EmitVarDecl(*VD);
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=342666=342665=342666=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Sep 20 10:19:41 2018
@@ -6555,7 +6555,7 @@ void OMPClauseWriter::VisitOMPOrderedCla
for (Expr *NumIter : C->getLoopNumIterations())
Record.AddStmt(NumIter);
for (unsigned I = 0, E = C->getLoopNumIterations().size(); I getLoopCunter(I));
+Record.AddStmt(C->getLoopCounter(I));
Record.AddSourceLocation(C->getLParenLoc());
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51926: [scan-build-py] Prevent crashes of CTU analysis by suppressing warnings
george.karpenkov accepted this revision. george.karpenkov added a comment. This revision is now accepted and ready to land. If that helps you, then sure. I'm not sure I understand why having warnings causes the collection process to fail, but I guess ultimately it's not important. Repository: rC Clang https://reviews.llvm.org/D51926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52280: Don't trim non-alphanumeric characters in 'file not found' errors for include directives.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC342667: r342177 introduced a hint in cases where an
#included file is not found. It… (authored by echristo, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D52280?vs=166184=166327#toc
Repository:
rC Clang
https://reviews.llvm.org/D52280
Files:
lib/Lex/PPDirectives.cpp
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1887,8 +1887,8 @@
// Check for likely typos due to leading or trailing non-isAlphanumeric
// characters
+ StringRef OriginalFilename = Filename;
if (!File) {
-StringRef OriginalFilename = Filename;
while (!isAlphanumeric(Filename.front())) {
Filename = Filename.drop_front();
}
@@ -1915,7 +1915,7 @@
// If the file is still not found, just go with the vanilla diagnostic
if (!File)
-Diag(FilenameTok, diag::err_pp_file_not_found) << Filename
+Diag(FilenameTok, diag::err_pp_file_not_found) << OriginalFilename
<< FilenameRange;
}
}
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1887,8 +1887,8 @@
// Check for likely typos due to leading or trailing non-isAlphanumeric
// characters
+ StringRef OriginalFilename = Filename;
if (!File) {
-StringRef OriginalFilename = Filename;
while (!isAlphanumeric(Filename.front())) {
Filename = Filename.drop_front();
}
@@ -1915,7 +1915,7 @@
// If the file is still not found, just go with the vanilla diagnostic
if (!File)
-Diag(FilenameTok, diag::err_pp_file_not_found) << Filename
+Diag(FilenameTok, diag::err_pp_file_not_found) << OriginalFilename
<< FilenameRange;
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52047: [clangd] Add building benchmark and memory consumption tracking
ilya-biryukov added a comment.
Also not sure about the trick:
- Would be surprising to see the "ms" instead of "mbytes"
- Time tends to vary between runs, so using Google Benchmark's capabilities to
run multiple times and report aggregated times seems useful. Not so much for
the memory usage: it's deterministic and running multiple times is just a waste
of time.
I wonder if a separate (and trivial) C++ binary that would load the index and
report the index size is any worse than the benchmark hack? What are the pros
of the latter?
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:81
+// Same for the next "benchmark".
+// FIXME(kbobyrev): Should this be separated into the BackingMemorySize
+// (underlying SymbolSlab size) and Symbol Index (MemIndex/Dex) overhead?
Given the trick we use for display, how are we going to show **two** memory
uses?
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:154
::benchmark::RunSpecifiedBenchmarks();
+ return 0;
}
Since `return 0` is implied for main in C++, there's no need to add one.
May add clarity though, so feel free to keep it!
Comment at: clang-tools-extra/clangd/index/SymbolYAML.cpp:189
+ if (!Buffer) {
+llvm::errs() << "Can't open " << SymbolFilename << "\n";
+return llvm::None;
Return `llvm::Expected` instead of `Optional` and create errors with the
specified text instead.
This is a slightly better option for the library code (callers can report
errors in various ways, e.g. one can imagine reporting it in the LSP instead of
stderr)
Comment at: clang-tools-extra/clangd/index/SymbolYAML.cpp:200
+else
+ llvm::errs() << "Bad RIFF: " << llvm::toString(RIFF.takeError()) << "\n";
+ } else {
Same here: just propagate the error to the caller.
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:239
for (const auto : InvertedIndex)
-Bytes += P.second.bytes();
+Bytes += P.first.Data.size() + P.second.bytes();
return Bytes + BackingDataSize;
Just to clarify: `P.first.Data.size()` is the size of the arena for all the
symbols?
https://reviews.llvm.org/D52047
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r342648 - [OPENMP] Add support for mapping memory pointed by member pointer.
Author: abataev
Date: Thu Sep 20 06:54:02 2018
New Revision: 342648
URL: http://llvm.org/viewvc/llvm-project?rev=342648=rev
Log:
[OPENMP] Add support for mapping memory pointed by member pointer.
Added support for map(s, s.ptr[0:1]) kind of mapping.
Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/test/OpenMP/target_map_codegen.cpp
cfe/trunk/test/OpenMP/target_map_messages.cpp
Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp?rev=342648=342647=342648=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp Thu Sep 20 06:54:02 2018
@@ -6752,7 +6752,9 @@ private:
MapBaseValuesArrayTy , MapValuesArrayTy ,
MapValuesArrayTy , MapFlagsArrayTy ,
StructRangeInfoTy , bool IsFirstComponentList,
- bool IsImplicit) const {
+ bool IsImplicit,
+ ArrayRef
+ OverlappedElements = llvm::None) const {
// The following summarizes what has to be generated for each map and the
// types below. The generated information is expressed in this order:
// base pointer, section pointer, size, flags
@@ -7023,7 +7025,6 @@ private:
Address LB =
CGF.EmitOMPSharedLValue(I->getAssociatedExpression()).getAddress();
-llvm::Value *Size = getExprTypeSize(I->getAssociatedExpression());
// If this component is a pointer inside the base struct then we don't
// need to create any entry for it - it will be combined with the
object
@@ -7032,6 +7033,70 @@ private:
IsPointer && EncounteredME &&
(dyn_cast(I->getAssociatedExpression()) ==
EncounteredME);
+if (!OverlappedElements.empty()) {
+ // Handle base element with the info for overlapped elements.
+ assert(!PartialStruct.Base.isValid() && "The base element is set.");
+ assert(Next == CE &&
+ "Expected last element for the overlapped elements.");
+ assert(!IsPointer &&
+ "Unexpected base element with the pointer type.");
+ // Mark the whole struct as the struct that requires allocation on
the
+ // device.
+ PartialStruct.LowestElem = {0, LB};
+ CharUnits TypeSize = CGF.getContext().getTypeSizeInChars(
+ I->getAssociatedExpression()->getType());
+ Address HB = CGF.Builder.CreateConstGEP(
+ CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(LB,
+ CGF.VoidPtrTy),
+ TypeSize.getQuantity() - 1, CharUnits::One());
+ PartialStruct.HighestElem = {
+ std::numeric_limits::max(),
+ HB};
+ PartialStruct.Base = BP;
+ // Emit data for non-overlapped data.
+ OpenMPOffloadMappingFlags Flags =
+ OMP_MAP_MEMBER_OF |
+ getMapTypeBits(MapType, MapTypeModifier, IsImplicit,
+ /*AddPtrFlag=*/false,
+ /*AddIsTargetParamFlag=*/false);
+ LB = BP;
+ llvm::Value *Size = nullptr;
+ // Do bitcopy of all non-overlapped structure elements.
+ for (OMPClauseMappableExprCommon::MappableExprComponentListRef
+ Component : OverlappedElements) {
+Address ComponentLB = Address::invalid();
+for (const OMPClauseMappableExprCommon::MappableComponent :
+ Component) {
+ if (MC.getAssociatedDeclaration()) {
+ComponentLB =
+CGF.EmitOMPSharedLValue(MC.getAssociatedExpression())
+.getAddress();
+Size = CGF.Builder.CreatePtrDiff(
+CGF.EmitCastToVoidPtr(ComponentLB.getPointer()),
+CGF.EmitCastToVoidPtr(LB.getPointer()));
+break;
+ }
+}
+BasePointers.push_back(BP.getPointer());
+Pointers.push_back(LB.getPointer());
+Sizes.push_back(Size);
+Types.push_back(Flags);
+LB = CGF.Builder.CreateConstGEP(ComponentLB, 1,
+CGF.getPointerSize());
+ }
+ BasePointers.push_back(BP.getPointer());
+ Pointers.push_back(LB.getPointer());
+ Size = CGF.Builder.CreatePtrDiff(
+ CGF.EmitCastToVoidPtr(
+ CGF.Builder.CreateConstGEP(HB, 1, CharUnits::One())
+ .getPointer()),
+ CGF.EmitCastToVoidPtr(LB.getPointer()));
+ Sizes.push_back(Size);
+ Types.push_back(Flags);
+ break;
+}
+llvm::Value *Size = getExprTypeSize(I->getAssociatedExpression());
if (!IsMemberPointer) {
[PATCH] D52261: [Sema] Generate completion fix-its for C code as well
ilya-biryukov added a comment. In https://reviews.llvm.org/D52261#1240143, @yvvan wrote: > I tried that first but did not I find a way just to copy an expression (we > basically need the same expr for such case). Do you know how to properly > generate a copy of expression or some other way to get the same expression? It seems `Sema::ActOnStartCXXMemberReference` only changes expression when overloading for C++'s `operator ->` is required, otherwise it keeps the same expression. Since C does not have that, we can just leave the expression as is. So setting `CorrectedLHS = LHS` for C should do the trick (no need to copy the expression IIUC, it's fine to use the same pointer for both `CorrectedLHS` and `LHS`). https://reviews.llvm.org/D52261 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added inline comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:67
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
shuaiwang wrote:
> JonasToth wrote:
> > That doesn't look like a super idea. It is super hidden that variable 'p*'
> > will be analyzed for pointee mutation and others aren't. Especially in 12
> > months and when someone else wants to change something, this will be the
> > source of headaches.
> >
> > Don't you think there could be a better way of doing this switch?
> Yeah... agree :)
> But how about let's keep it this way just for now, and I'll pause the work on
> supporting pointee analysis and fix it properly but in a separate diff
> following this one?
>
> I've been thinking about this and also did some prototyping, and here are my
> thoughts:
> In order to properly fix this we need to change the interface of
> `ExprMutationAnalyzer` to not simply return a `Stmt` but instead return a
> `MutationTrace` with additional metadata. This wasn't needed before because
> we can reconstruct a mutation chain by continuously calling `findMutation`,
> and that works well for cases like "int x; int& r0 = x; int& r1 = r0; r1 =
> 123;". But now that pointers come into play, and we need to handle cases like
> "int *p; int *p2 = p; int& r = *p2; r = 123;", and in order to reconstruct
> the mutation chain, we need to do `findPointeeMutation(p)` ->
> `findPointeeMutation(p2)` -> `findMutation(r)`, notice that we need to switch
> from calling `findPointeeMutation` to calling `findMutation`. However we
> don't know where the switch should happen simply based on what's returned
> from `findPointeeMutation`, we need additional metadata for that.
>
> That interface change will unavoidably affect how memoization is done so we
> need to change memoization as well.
>
> Now since we need to change both the interface and memoization anyway, we
> might as well design them properly so that multiple-layers of pointers can be
> supported nicely. I think we can end up with something like this:
> ```
> class ExprMutationAnalyzer {
> public:
> struct MutationTrace {
> const Stmt *Value;
> const MutationTrace *Next;
> // Other metadata
> };
>
> // Finds whether any mutative operations are performed on the given
> expression after dereferencing `DerefLayers` times.
> const MutationTrace *findMutation(const Expr *, int DerefLayers = 0);
> const MutationTrace *findMutation(const Decl *, int DerefLayers = 0);
> };
> ```
> This involves quite some refactoring, and doing this refactoring before this
> diff and later rebasing seems quite some trouble that I'd like to avoid.
>
> Let me know what do you think :)
Is the second part of your answer part to adressing this testing issue?
Given the whole Analyzer is WIP it is ok to postpone the testing change to
later for me. But I feel that this is a quality issue that more experience
clang develops should comment on because it still lands in trunk. Are you aware
of other patches then clang-tidy that rely on EMA?
Regarding you second part (its related to multi-layer pointer mutation?!):
Having a dependency-graph that follows the general data flow _with_ mutation
annotations would be nice to have. There is probably even something in clang
(e.g. `CFG` is probably similar in idea, just with all execution paths), but I
don't know enough to properly judge here.
In my opinion it is ok, to not do the refactoring now and just support one
layer of pointer indirection. Especially in C++ there is usually no need for
multi-layer pointers but more a relict from C-style.
C on the other hand relies on that.
Designing EMA new for the more generalized functionality (if necessary) would
be of course great, but again: Better analyzer ppl should be involved then me,
even though I would still be very interested to help :)
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
JonasToth wrote:
> I feel that there a multiple tests missing:
>
> - multiple levels of pointers `int ***`, `int * const *`
> - pointers to references `int &*`
> - references to pointers `int *&`
> - ensure that having a const pointer does no influence the pointee analysis
> `int * const p = *p = 42;`
> - a class with `operator*` + `operator->` const/non-const and the analysis
> for pointers to that class
> - pointer returned from a function
> - non-const reference returned
> ```
> int& foo(int *p) {
> return *p;
> }
> ```
>
for the multi-level pointer mutation: it would be enough to test, that the
second layer is analyzed properly, and that the `int * >const< *` would be
detected.
Repository:
rC Clang
https://reviews.llvm.org/D52219
[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy
lebedev.ri added a comment. This looks questionable to me. I don't disagree with the reasons stated about llvm types. But is that *always* true? I would personally be very surprized, and consider this a false-positive. This should at least be optional. Not sure about the default, but setting the `StrictMode` should certainly disable this relaxation. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52315 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r229575 - clang-cl: Disable frame pointer elimination at -O0
It looks like we don't do anything special if you run clang-cl -O0 or /O0, but it's not an error. I don't have my computer and can't run a test, but from the outside, it looks like clang-cl -O0 does generate unoptimized code without warning about an unrecognized flag, but it doesn't disable FP elimination. It's also a GCC style flag, and usually if a core option is accepted, it has the same behavior in both driver modes. I'd be fine removing this extra handling so long as the driver explicitly tells the user that /O0 isn't recognized. While we're at it, maybe we should warn about other unrecognized /O flags. On Wed, Sep 19, 2018 at 6:10 PM Nico Weber wrote: > The generic O flag handling doesn't support 0 either. Would you be ok with > removing this? > > Does /Od do what you want? > > On Wed, Sep 19, 2018 at 4:52 PM Reid Kleckner > wrote: > >> I was probably using it myself, and was surprised that /O0 and -O0 had >> different behavior, because -O0 will hit the special O0 flag that disables >> FPO, but /O0 will hit the generic 'O' flag handling. >> >> On Wed, Sep 19, 2018 at 7:10 AM Nico Weber wrote: >> >>> Do you remember why you landed this? cl.exe doesn't support /O0; why >>> does clang-cl? >>> >>> On Tue, Feb 17, 2015 at 5:53 PM Reid Kleckner wrote: >>> Author: rnk Date: Tue Feb 17 16:40:42 2015 New Revision: 229575 URL: http://llvm.org/viewvc/llvm-project?rev=229575=rev Log: clang-cl: Disable frame pointer elimination at -O0 This wasn't kicking in because the _SLASH_O flag didn't match our check for OPT_O0. Add an alias that does to keep the logic simple. Modified: cfe/trunk/include/clang/Driver/CLCompatOptions.td Modified: cfe/trunk/include/clang/Driver/CLCompatOptions.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CLCompatOptions.td?rev=229575=229574=229575=diff == --- cfe/trunk/include/clang/Driver/CLCompatOptions.td (original) +++ cfe/trunk/include/clang/Driver/CLCompatOptions.td Tue Feb 17 16:40:42 2015 @@ -80,6 +80,7 @@ def _SLASH_I : CLJoinedOrSeparate<"I">, Alias; def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">, Alias; +def _SLASH_O0 : CLFlag<"O0">, Alias; def _SLASH_O : CLJoined<"O">, HelpText<"Optimization level">, MetaVarName<"">, Alias; def _SLASH_Ob0 : CLFlag<"Ob0">, HelpText<"Disable inlining">, ___ cfe-commits mailing list cfe-comm...@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)
riccibruno updated this revision to Diff 166324.
riccibruno marked 9 inline comments as done.
riccibruno added a comment.
Address rjmccall comments:
1. Renamed `CXXSpecialName` to `CXXSpecialNameExtra`
2. Introduced a constant `IdentifierInfoAlignment` for
`alignas(IdentifierInfoAlignment)`
3. Updated some comments
4. Cleaned up the `static_assert` which checked the consistency of the
numerical values of the enumerators.
Regarding using a `PointerIntPair` it would work if we pass a custom
`PointerLikeTypeTraits` to state that, yes, our `void *` are aligned to 8 bytes
instead of 4. However I am not sure this is a good idea since the
`PointerIntPair`
do not really simplify the code. What we really want here is a pointer union
but we
also need precise control over the low bits.
Repository:
rC Clang
https://reviews.llvm.org/D52267
Files:
include/clang/AST/DeclarationName.h
include/clang/Basic/IdentifierTable.h
lib/AST/DeclarationName.cpp
lib/Basic/IdentifierTable.cpp
lib/Sema/IdentifierResolver.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTWriter.cpp
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -3574,7 +3574,7 @@
II->isPoisoned() ||
(IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) ||
II->hasRevertedTokenIDToIdentifier() ||
-(NeedDecls && II->getFETokenInfo()))
+(NeedDecls && II->getFETokenInfo()))
return true;
return false;
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -908,7 +908,7 @@
(IsModule ? II.hasRevertedBuiltin() : II.getObjCOrBuiltinID()) ||
II.hasRevertedTokenIDToIdentifier() ||
(!(IsModule && Reader.getPreprocessor().getLangOpts().CPlusPlus) &&
- II.getFETokenInfo());
+ II.getFETokenInfo());
}
static bool readBit(unsigned ) {
Index: lib/Sema/IdentifierResolver.cpp
===
--- lib/Sema/IdentifierResolver.cpp
+++ lib/Sema/IdentifierResolver.cpp
@@ -147,7 +147,7 @@
if (IdentifierInfo *II = Name.getAsIdentifierInfo())
updatingIdentifier(*II);
- void *Ptr = Name.getFETokenInfo();
+ void *Ptr = Name.getFETokenInfo();
if (!Ptr) {
Name.setFETokenInfo(D);
@@ -172,7 +172,7 @@
if (IdentifierInfo *II = Name.getAsIdentifierInfo())
updatingIdentifier(*II);
- void *Ptr = Name.getFETokenInfo();
+ void *Ptr = Name.getFETokenInfo();
if (!Ptr) {
AddDecl(D);
@@ -213,7 +213,7 @@
if (IdentifierInfo *II = Name.getAsIdentifierInfo())
updatingIdentifier(*II);
- void *Ptr = Name.getFETokenInfo();
+ void *Ptr = Name.getFETokenInfo();
assert(Ptr && "Didn't find this decl on its identifier's chain!");
@@ -232,7 +232,7 @@
if (IdentifierInfo *II = Name.getAsIdentifierInfo())
readingIdentifier(*II);
- void *Ptr = Name.getFETokenInfo();
+ void *Ptr = Name.getFETokenInfo();
if (!Ptr) return end();
if (isDeclPtr(Ptr))
@@ -304,7 +304,7 @@
if (IdentifierInfo *II = Name.getAsIdentifierInfo())
readingIdentifier(*II);
- void *Ptr = Name.getFETokenInfo();
+ void *Ptr = Name.getFETokenInfo();
if (!Ptr) {
Name.setFETokenInfo(D);
@@ -397,7 +397,7 @@
/// It creates a new IdDeclInfo if one was not created before for this id.
IdentifierResolver::IdDeclInfo &
IdentifierResolver::IdDeclInfoMap::operator[](DeclarationName Name) {
- void *Ptr = Name.getFETokenInfo();
+ void *Ptr = Name.getFETokenInfo();
if (Ptr) return *toIdDeclInfo(Ptr);
@@ -415,7 +415,7 @@
void IdentifierResolver::iterator::incrementSlowCase() {
NamedDecl *D = **this;
- void *InfoPtr = D->getDeclName().getFETokenInfo();
+ void *InfoPtr = D->getDeclName().getFETokenInfo();
assert(!isDeclPtr(InfoPtr) && "Decl with wrong id ?");
IdDeclInfo *Info = toIdDeclInfo(InfoPtr);
Index: lib/Basic/IdentifierTable.cpp
===
--- lib/Basic/IdentifierTable.cpp
+++ lib/Basic/IdentifierTable.cpp
@@ -382,50 +382,49 @@
namespace clang {
-/// MultiKeywordSelector - One of these variable length records is kept for each
+/// One of these variable length records is kept for each
/// selector containing more than one keyword. We use a folding set
/// to unique aggregate names (keyword selectors in ObjC parlance). Access to
/// this class is provided strictly through Selector.
-class MultiKeywordSelector
- : public DeclarationNameExtra, public llvm::FoldingSetNode {
- MultiKeywordSelector(unsigned nKeys) {
-ExtraKindOrNumArgs = NUM_EXTRA_KINDS + nKeys;
- }
+class alignas(IdentifierInfoAlignment) MultiKeywordSelector
+: public detail::DeclarationNameExtra,
+ public llvm::FoldingSetNode {
+
[PATCH] D46140: [coroutines] std::task type (WIP)
lewissbaker added inline comments. Comment at: test/std/experimental/task/sync_wait.hpp:36-37 + __isSet_ = true; +} +__cv_.notify_all(); + } The call to `notify_all()` needs to be inside the lock. Otherwise, it's possible the waiting thread may see the write to `__isSet_` inside `wait()` below and return, destroying the `condition_variable` before `__cv_.notify_all()` call completes. https://reviews.llvm.org/D46140 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)
riccibruno added a comment.
Addressed some inline comments.
Comment at: include/clang/AST/DeclarationName.h:46
-/// DeclarationName - The name of a declaration. In the common case,
-/// this just stores an IdentifierInfo pointer to a normal
-/// name. However, it also provides encodings for Objective-C
-/// selectors (optimizing zero- and one-argument selectors, which make
-/// up 78% percent of all selectors in Cocoa.h) and special C++ names
-/// for constructors, destructors, and conversion functions.
+namespace detail {
+
rjmccall wrote:
> Should `DeclarationNameExtra` be moved here? I'm not sure why it's in
> `IdentifierTable.h` in the first place, and your refactor seems to make that
> even more pointless.
`DeclarationNameExtra` is unfortunately needed by `MultiKeywordSelector`
in `IdentifierInfo.cpp`.
Comment at: include/clang/AST/DeclarationName.h:164
+CXXUsingDirective = 10,
+ObjCMultiArgSelector = 11
+ };
rjmccall wrote:
> Is the criterion for inclusion in the first seven really just frequency of
> use, or is it a combination of that and relative benefit?
>
> The only one I would really quibble about is that multi-argument selectors
> are probably more common and important to Objective-C code than conversion
> operators are to C++. But it's quite possible that the relative benefit is
> much higher for C++, since selectors only appear on specific kinds of
> declarations that know they're declared with selectors — relatively little
> code actually needs to be polymorphic about them — and since they have to be
> defined out-of-line.
I did not do an in-depth analysis regarding the selection of the first seven
inline kinds.
My thought process was that C++ constructors and operator names should
definitely be
inline. C++ destructors seemed much more frequent than c++ literal operators and
deduction guides. This leaves one slot available and since C++ conversion
operators
share the same class `CXXSpecialName` including it as an inline kind simplifies
the code.
Also a practical point is that I don't really have a representative sample of
ObjC code
to benchmark this. For C++ I am using all of Boost which I hope is
representative
enough. If you deem it necessary I will try to do some benchmarks with
`ObjCMultiArgSelector` stored inline.
Repository:
rC Clang
https://reviews.llvm.org/D52267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy
flx added inline comments. Comment at: clang-tidy/utils/TypeTraits.cpp:49 + if (Context.getTypeSize(Type) <= Context.getTypeSize(Context.VoidPtrTy)) +return false; + This early return now ignores the fact that the type has non-trivial copy constructors, virtual destructors etc which makes the type expensive to copy. Could we achieve the same desired outcome by adding a blacklist that allows users exclude types they deem to be not expensive? This way they would get a warning the first time they run the check and then could disable such types. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52315 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
