[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet,
baloghadamsoftware, whisperity.
Previously, the checker only tracked one raw pointer symbol for each container
object. But member functions returning a pointer to the object's inner buffer
may be called on the object several times.
These pointer symbols are now collected in a set inside the program state map
and thus all of them is checked for use-after-free problems.
Repository:
rC Clang
https://reviews.llvm.org/D49057
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -108,6 +108,28 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
+void multiple_symbols(bool Cond) {
+ const char *c1, *d1;
+ {
+std::string s1;
+c1 = s1.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+d1 = s1.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+const char *local = s1.c_str();
+consume(local); // no-warning
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
+ // expected-note@-1 {{Internal buffer is released because the object was destroyed}}
+ std::string s2;
+ const char *c2 = s2.c_str();
+ if (Cond) {
+// expected-note@-1 {{Assuming 'Pred' is not equal to 0}} // expected-note@-1 {{Taking true branch}}
+// expected-note@-2 {{Assuming 'Pred' is 0}} // expected-note@-2 {{Taking false branch}}
+consume(c1); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d1); // expected-warning {{Use of memory after it is freed}}
+ } // expected-note@-1 {{Use of memory after it is freed}}
+}
+
void deref_after_scope_cstr_ok() {
const char *c;
std::string s;
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -24,10 +24,22 @@
using namespace clang;
using namespace ento;
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+typedef llvm::ImmutableSet PtrSet;
+
+// Associate container objects with a set of raw pointer symbols.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
+
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+ : public ProgramStatePartialTrait {
+ static void *GDMIndex() {
+static int Index = 0;
+return &Index;
+ }
+};
+} // end namespace ento
+} // end namespace clang
namespace {
@@ -60,8 +72,8 @@
// lookup by region.
bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
RawPtrMapTy Map = State->get();
- for (const auto Entry : Map) {
-if (Entry.second == Sym)
+ for (const auto &Entry : Map) {
+if (Entry.second.contains(Sym))
return true;
}
return false;
@@ -88,32 +100,45 @@
return;
SVal Obj = ICall->getCXXThisVal();
- const auto *TypedR = dyn_cast_or_null(Obj.getAsRegion());
- if (!TypedR)
+ const auto *ObjRegion = dyn_cast_or_null(Obj.getAsRegion());
+ if (!ObjRegion)
return;
- auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+ auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
if (TypeDecl->getName() != "basic_string")
return;
ProgramStateRef State = C.getState();
if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
- State = State->set(TypedR, RawPtr.getAsSymbol());
- C.addTransition(State);
+ // Start tracking this raw pointer by adding it to the set of symbols
+ // associated with this container object in the program state map.
+ PtrSet::Factory &F = State->getStateManager().get_context();
+ PtrSet NewSet = F.getEmptySet();
+ if (State->contains(ObjRegion)) {
+NewSet = *State->get(ObjRegion);
+if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
+ }
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+State = State->set(ObjRegion, NewSet);
+C.addTransition(State);
+ }
}
return;
}
if (isa(ICall)) {
-if (State->contains(TypedR)) {
- const SymbolRef *StrBufferPtr
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.
Overall looks good, some nits inline. Let's run it on some projects to exercise
this change.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:27
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+typedef llvm::ImmutableSet PtrSet;
+
I think we should use `using` now instead of `typedef`.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+State = State->set(ObjRegion, NewSet);
Is it possible here the set to be empty? We just added a new element to it
above. Maybe turn this into an assert or just omit this if it is impossible?
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:138
const Expr *Origin = Call.getOriginExpr();
- State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
- State = State->remove(TypedR);
+ const PtrSet *PS = State->get(ObjRegion);
+ for (const auto &Symbol : *PS)
Using both contains and get will result in two lookups. Maybe it would be
better to just use get and check if the result is nullptr?
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:159
}
+if (State->contains(Entry.first)) {
+ const PtrSet *OldSet = State->get(Entry.first);
Same comment as above.
Comment at: test/Analysis/dangling-internal-buffer.cpp:111
+void multiple_symbols(bool Cond) {
+ const char *c1, *d1;
We started to use lowercase letters for variable names. Maybe this is not the
best, since it is not following the LLVM coding guidelines. So I think it would
be better to rename this `Cond` to start with a lowercase letter in this patch
for consistency, and update the tests to follow the LLVM coding styleguide in a
separate patch later.
Repository:
rC Clang
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48712: [X86] Lowering integer truncation intrinsics to native IR
RKSimon added a comment. LGTM - @craig.topper any comments? https://reviews.llvm.org/D48712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48712: [X86] Lowering integer truncation intrinsics to native IR
craig.topper accepted this revision. craig.topper added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D48712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs updated this revision to Diff 154519.
rnkovacs marked 5 inline comments as done.
rnkovacs edited the summary of this revision.
rnkovacs added a comment.
Addressed comments.
https://reviews.llvm.org/D49057
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -108,6 +108,30 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
+void multiple_symbols(bool cond) {
+ const char *c1, *d1;
+ {
+std::string s1;
+c1 = s1.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+d1 = s1.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+const char *local = s1.c_str();
+consume(local); // no-warning
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
+ // expected-note@-1 {{Internal buffer is released because the object was destroyed}}
+ std::string s2;
+ const char *c2 = s2.c_str();
+ if (cond) {
+// expected-note@-1 {{Assuming 'cond' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'cond' is 0}}
+// expected-note@-4 {{Taking false branch}}
+consume(c1); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d1); // expected-warning {{Use of memory after it is freed}}
+ } // expected-note@-1 {{Use of memory after it is freed}}
+}
+
void deref_after_scope_cstr_ok() {
const char *c;
std::string s;
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -24,10 +24,22 @@
using namespace clang;
using namespace ento;
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+using PtrSet = llvm::ImmutableSet;
+
+// Associate container objects with a set of raw pointer symbols.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
+
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+ : public ProgramStatePartialTrait {
+ static void *GDMIndex() {
+static int Index = 0;
+return &Index;
+ }
+};
+} // end namespace ento
+} // end namespace clang
namespace {
@@ -60,8 +72,8 @@
// lookup by region.
bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
RawPtrMapTy Map = State->get();
- for (const auto Entry : Map) {
-if (Entry.second == Sym)
+ for (const auto &Entry : Map) {
+if (Entry.second.contains(Sym))
return true;
}
return false;
@@ -88,32 +100,43 @@
return;
SVal Obj = ICall->getCXXThisVal();
- const auto *TypedR = dyn_cast_or_null(Obj.getAsRegion());
- if (!TypedR)
+ const auto *ObjRegion = dyn_cast_or_null(Obj.getAsRegion());
+ if (!ObjRegion)
return;
- auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+ auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
if (TypeDecl->getName() != "basic_string")
return;
ProgramStateRef State = C.getState();
if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
- State = State->set(TypedR, RawPtr.getAsSymbol());
+ // Start tracking this raw pointer by adding it to the set of symbols
+ // associated with this container object in the program state map.
+ PtrSet::Factory &F = State->getStateManager().get_context();
+ PtrSet NewSet = F.getEmptySet();
+ if (State->contains(ObjRegion)) {
+NewSet = *State->get(ObjRegion);
+if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
+ }
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ assert(!NewSet.isEmpty());
+ State = State->set(ObjRegion, NewSet);
C.addTransition(State);
}
return;
}
if (isa(ICall)) {
-if (State->contains(TypedR)) {
- const SymbolRef *StrBufferPtr = State->get(TypedR);
- // FIXME: What if Origin is null?
+if (const PtrSet *PS = State->get(ObjRegion)) {
+ // Mark all pointer symbols associated with the deleted object released.
const Expr *Origin = Call.getOriginExpr();
- State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
- State = State->remove(TypedR);
+ for (const auto &Symbol : *PS)
+Sta
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+State = State->set(ObjRegion, NewSet);
xazax.hun wrote:
> Is it possible here the set to be empty? We just added a new element to it
> above. Maybe turn this into an assert or just omit this if it is impossible?
I'm not sure whether `add()` can fail. I turned it into an assert now and will
see if it ever fails.
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
xazax.hun added a comment.
Thanks! The changes look good, I forgot to mark one double lookup though in my
previous review.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:121
+ if (State->contains(ObjRegion)) {
+NewSet = *State->get(ObjRegion);
+if (NewSet.contains(RawPtr.getAsSymbol()))
Oh, there is also a double lookup here, sorry I did not spot it in the initial
review.
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs updated this revision to Diff 154520.
rnkovacs marked an inline comment as done.
https://reviews.llvm.org/D49057
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -108,6 +108,30 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
+void multiple_symbols(bool cond) {
+ const char *c1, *d1;
+ {
+std::string s1;
+c1 = s1.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+d1 = s1.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+const char *local = s1.c_str();
+consume(local); // no-warning
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
+ // expected-note@-1 {{Internal buffer is released because the object was destroyed}}
+ std::string s2;
+ const char *c2 = s2.c_str();
+ if (cond) {
+// expected-note@-1 {{Assuming 'cond' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'cond' is 0}}
+// expected-note@-4 {{Taking false branch}}
+consume(c1); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d1); // expected-warning {{Use of memory after it is freed}}
+ } // expected-note@-1 {{Use of memory after it is freed}}
+}
+
void deref_after_scope_cstr_ok() {
const char *c;
std::string s;
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -24,10 +24,22 @@
using namespace clang;
using namespace ento;
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+using PtrSet = llvm::ImmutableSet;
+
+// Associate container objects with a set of raw pointer symbols.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
+
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+ : public ProgramStatePartialTrait {
+ static void *GDMIndex() {
+static int Index = 0;
+return &Index;
+ }
+};
+} // end namespace ento
+} // end namespace clang
namespace {
@@ -61,7 +73,7 @@
bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
RawPtrMapTy Map = State->get();
for (const auto Entry : Map) {
-if (Entry.second == Sym)
+if (Entry.second.contains(Sym))
return true;
}
return false;
@@ -88,32 +100,43 @@
return;
SVal Obj = ICall->getCXXThisVal();
- const auto *TypedR = dyn_cast_or_null(Obj.getAsRegion());
- if (!TypedR)
+ const auto *ObjRegion = dyn_cast_or_null(Obj.getAsRegion());
+ if (!ObjRegion)
return;
- auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+ auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
if (TypeDecl->getName() != "basic_string")
return;
ProgramStateRef State = C.getState();
if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
- State = State->set(TypedR, RawPtr.getAsSymbol());
+ // Start tracking this raw pointer by adding it to the set of symbols
+ // associated with this container object in the program state map.
+ PtrSet::Factory &F = State->getStateManager().get_context();
+ PtrSet NewSet = F.getEmptySet();
+ if (const PtrSet *OldSet = State->get(ObjRegion)) {
+NewSet = *OldSet;
+if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
+ }
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ assert(!NewSet.isEmpty());
+ State = State->set(ObjRegion, NewSet);
C.addTransition(State);
}
return;
}
if (isa(ICall)) {
-if (State->contains(TypedR)) {
- const SymbolRef *StrBufferPtr = State->get(TypedR);
- // FIXME: What if Origin is null?
+if (const PtrSet *PS = State->get(ObjRegion)) {
+ // Mark all pointer symbols associated with the deleted object released.
const Expr *Origin = Call.getOriginExpr();
- State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
- State = State->remove(TypedR);
+ for (const auto Symbol : *PS)
+State = allocation_state::markReleased(State, Symbol, Origin);
+ State = State->remove(ObjRegion);
C.addTransition(State);
return;
}
@@
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:121
+ if (State->contains(ObjRegion)) {
+NewSet = *State->get(ObjRegion);
+if (NewSet.contains(RawPtr.getAsSymbol()))
xazax.hun wrote:
> Oh, there is also a double lookup here, sorry I did not spot it in the
> initial review.
Thanks for noticing. I should have seen it in the first place :)
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49058: [analyzer] Move DanglingInternalBufferChecker out of alpha
rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet,
baloghadamsoftware, whisperity.
Repository:
rC Clang
https://reviews.llvm.org/D49058
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -276,6 +276,11 @@
let ParentPackage = Cplusplus in {
+def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
+ HelpText<"Reports the usage of internal raw pointers of C++ containers "
+ "after invalidation.">,
+ DescFile<"DanglingInternalBufferChecker.cpp">;
+
def NewDeleteChecker : Checker<"NewDelete">,
HelpText<"Check for double-free and use-after-free problems. Traces memory
managed by new/delete.">,
DescFile<"MallocChecker.cpp">;
@@ -300,11 +305,6 @@
let ParentPackage = CplusplusAlpha in {
-def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
- HelpText<"Check for internal raw pointers of C++ standard library containers
"
- "used after deallocation">,
- DescFile<"DanglingInternalBufferChecker.cpp">;
-
def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
HelpText<"Reports destructions of polymorphic objects with a non-virtual "
"destructor in their base class">,
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -276,6 +276,11 @@
let ParentPackage = Cplusplus in {
+def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
+ HelpText<"Reports the usage of internal raw pointers of C++ containers "
+ "after invalidation.">,
+ DescFile<"DanglingInternalBufferChecker.cpp">;
+
def NewDeleteChecker : Checker<"NewDelete">,
HelpText<"Check for double-free and use-after-free problems. Traces memory managed by new/delete.">,
DescFile<"MallocChecker.cpp">;
@@ -300,11 +305,6 @@
let ParentPackage = CplusplusAlpha in {
-def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
- HelpText<"Check for internal raw pointers of C++ standard library containers "
- "used after deallocation">,
- DescFile<"DanglingInternalBufferChecker.cpp">;
-
def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
HelpText<"Reports destructions of polymorphic objects with a non-virtual "
"destructor in their base class">,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
NoQ added a comment.
Much symbols!
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+State = State->set(ObjRegion, NewSet);
rnkovacs wrote:
> xazax.hun wrote:
> > Is it possible here the set to be empty? We just added a new element to it
> > above. Maybe turn this into an assert or just omit this if it is impossible?
> I'm not sure whether `add()` can fail. I turned it into an assert now and
> will see if it ever fails.
It totally can't. We're just adding an object to a set. What could possibly go
wrong?
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:32-40
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+ : public ProgramStatePartialTrait {
+ static void *GDMIndex() {
+static int Index = 0;
+return &Index;
Please add a comment on how this template is useful. This trick is used by some
checkers, but it's a vry unobvious trick. We should probably add a macro
for that, i.e. `REGISTER_FACTORY_WITH_PROGRAMSTATE` or something like that.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:113
- const SymbolRef *StrBufferPtr = State->get(TypedR);
- // FIXME: What if Origin is null?
const Expr *Origin = Call.getOriginExpr();
Maybe turn the FIXME into a NOTE or something like that. It indeed seems fine,
but let's still make sure the reader realizes that there's a subtle potential
problem here.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:115
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
+ // Start tracking this raw pointer by adding it to the set of symbols
I'd much rather unwrap the symbol here, i.e. `if (SymbolRef Sym =
RawPtr.getAsSymbol())`. A lot of other cornercases may occur if the
implementation is accidentally inlined (`Undefined`, concrete regions). Also,
speculatively, `.getAsSymbol(/* search for symbolic base = */ true)` for the
same reason//*//.
If we didn't care about inlined implementations, the `Unknown` check would have
been redundant. So it should also be safe to straightforwardly ignore inlined
implementations by consulting `C.wasInlined`, then the presence of the symbol
can be asserted. But i'd rather speculatively care about inlined
implementations as long as it seems easy.
__
//*// In fact your code relies on a very wonky implementation detail of our
`SVal` hierarchy: namely, pointer-type return values of conservatively
evaluated functions are always expressed as `&SymRegion{conj_$N}`
and never as `&element{SymRegion{conj_$N}, 0 S32b, pointee
type}`. Currently nobody knows the rules under which zero element regions are
added in different parts of the analyzer, i.e. what is the "canonical"
representation of the symbolic pointer, though i made a few attempts to
speculate.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:119
+ PtrSet::Factory &F = State->getStateManager().get_context();
+ PtrSet NewSet = F.getEmptySet();
+ if (const PtrSet *OldSet = State->get(ObjRegion)) {
My favorite idiom for such cases, looks a bit cleaner:
```
const PtrSet *SetPtr = State->get(ObjRegion)
PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet()
```
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:122
+NewSet = *OldSet;
+if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
This `contains` check is very unlikely to yield true because if the
implementation is not inlined, the symbol is newly born. I'd much rather skip
the check; it doesn't sound like it'd make things any faster.
Even better, let's assert that: `assert(C.wasInlined || !Set.contains(Sym))`.
It'll probably help us catch some "reincarnated symbol" bugs in the core.
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49059: [MinGW] Skip adding default win32 api libraries if -lwindowsapp is specified
mstorsjo created this revision.
mstorsjo added reviewers: rnk, martell, compnerd, smeenai.
In this setup, skip adding all the default windows import libraries, if linking
to windowsapp (which replaces them, when targeting the windows store/UWP api
subset).
With GCC, the same is achieved by using a custom spec file, but since clang
doesn't use spec files, we have to allow other means of overriding what default
libraries to use (without going all the way to using -nostdlib, which would
exclude everything). The same approach, in detecting certain user specified
libraries and omitting others from the defaults, was already used in SVN
r314138.
Repository:
rC Clang
https://reviews.llvm.org/D49059
Files:
lib/Driver/ToolChains/MinGW.cpp
test/Driver/mingw-windowsapp.c
Index: test/Driver/mingw-windowsapp.c
===
--- /dev/null
+++ test/Driver/mingw-windowsapp.c
@@ -0,0 +1,6 @@
+// RUN: %clang -v -target i686-pc-windows-gnu -### %s 2>&1 | FileCheck
-check-prefix=CHECK_DEFAULT %s
+// RUN: %clang -v -target i686-pc-windows-gnu -### %s -lwindowsapp 2>&1 |
FileCheck -check-prefix=CHECK_WINDOWSAPP %s
+
+// CHECK_DEFAULT: "-lmsvcrt" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32"
"-lmingw32"
+// CHECK_WINDOWSAPP: "-lwindowsapp" "-lmingw32"
+// CHECK_WINDOWSAPP-SAME: "-lmsvcrt" "-lmingw32"
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -201,6 +201,11 @@
CmdArgs.push_back("-Bdynamic");
}
+ bool HasWindowsApp = false;
+ for (auto Lib : Args.getAllArgValues(options::OPT_l))
+if (Lib == "windowsapp")
+ HasWindowsApp = true;
+
if (!Args.hasArg(options::OPT_nostdlib)) {
if (!Args.hasArg(options::OPT_nodefaultlibs)) {
if (Args.hasArg(options::OPT_static))
@@ -223,15 +228,17 @@
if (Args.hasArg(options::OPT_pthread))
CmdArgs.push_back("-lpthread");
- // add system libraries
- if (Args.hasArg(options::OPT_mwindows)) {
-CmdArgs.push_back("-lgdi32");
-CmdArgs.push_back("-lcomdlg32");
+ if (!HasWindowsApp) {
+// add system libraries
+if (Args.hasArg(options::OPT_mwindows)) {
+ CmdArgs.push_back("-lgdi32");
+ CmdArgs.push_back("-lcomdlg32");
+}
+CmdArgs.push_back("-ladvapi32");
+CmdArgs.push_back("-lshell32");
+CmdArgs.push_back("-luser32");
+CmdArgs.push_back("-lkernel32");
}
- CmdArgs.push_back("-ladvapi32");
- CmdArgs.push_back("-lshell32");
- CmdArgs.push_back("-luser32");
- CmdArgs.push_back("-lkernel32");
if (Args.hasArg(options::OPT_static))
CmdArgs.push_back("--end-group");
Index: test/Driver/mingw-windowsapp.c
===
--- /dev/null
+++ test/Driver/mingw-windowsapp.c
@@ -0,0 +1,6 @@
+// RUN: %clang -v -target i686-pc-windows-gnu -### %s 2>&1 | FileCheck -check-prefix=CHECK_DEFAULT %s
+// RUN: %clang -v -target i686-pc-windows-gnu -### %s -lwindowsapp 2>&1 | FileCheck -check-prefix=CHECK_WINDOWSAPP %s
+
+// CHECK_DEFAULT: "-lmsvcrt" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32" "-lmingw32"
+// CHECK_WINDOWSAPP: "-lwindowsapp" "-lmingw32"
+// CHECK_WINDOWSAPP-SAME: "-lmsvcrt" "-lmingw32"
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -201,6 +201,11 @@
CmdArgs.push_back("-Bdynamic");
}
+ bool HasWindowsApp = false;
+ for (auto Lib : Args.getAllArgValues(options::OPT_l))
+if (Lib == "windowsapp")
+ HasWindowsApp = true;
+
if (!Args.hasArg(options::OPT_nostdlib)) {
if (!Args.hasArg(options::OPT_nodefaultlibs)) {
if (Args.hasArg(options::OPT_static))
@@ -223,15 +228,17 @@
if (Args.hasArg(options::OPT_pthread))
CmdArgs.push_back("-lpthread");
- // add system libraries
- if (Args.hasArg(options::OPT_mwindows)) {
-CmdArgs.push_back("-lgdi32");
-CmdArgs.push_back("-lcomdlg32");
+ if (!HasWindowsApp) {
+// add system libraries
+if (Args.hasArg(options::OPT_mwindows)) {
+ CmdArgs.push_back("-lgdi32");
+ CmdArgs.push_back("-lcomdlg32");
+}
+CmdArgs.push_back("-ladvapi32");
+CmdArgs.push_back("-lshell32");
+CmdArgs.push_back("-luser32");
+CmdArgs.push_back("-lkernel32");
}
- CmdArgs.push_back("-ladvapi32");
- CmdArgs.push_back("-lshell32");
- CmdArgs.push_back("-luser32");
- CmdArgs.push_back("-lkernel32");
if (Args.hasArg(options::OPT_static))
CmdArgs.push_back("--end-group");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
[PATCH] D47632: [ASTImporter] Refactor Decl creation
a_sidorin added a comment.
Hi Gabor,
I like the new syntax. There are some comments inline; most of them are just
stylish.
Comment at: lib/AST/ASTImporter.cpp:94
+ void updateAttrAndFlags(const Decl *From, Decl *To) {
+// check if some flags or attrs are new in 'From' and copy into 'To'
+// FIXME: other flags or attrs?
Comments should be complete sentences: start with a capital and end with a
period.
Comment at: lib/AST/ASTImporter.cpp:114
+
+// Always use theses functions to create a Decl during import. There are
+// certain tasks which must be done after the Decl was created, e.g. we
these?
Comment at: lib/AST/ASTImporter.cpp:161
+ ToD->IdentifierNamespace = FromD->IdentifierNamespace;
+ if (FromD->hasAttrs())
+for (const Attr *FromAttr : FromD->getAttrs())
Should we move the below operations into `updateAttrAndFlags()` and use it
instead?
Comment at: lib/AST/ASTImporter.cpp:1587
StructuralEquivalenceContext Ctx(
Importer.getFromContext(), Importer.getToContext(),
+ Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
(Thinking out loud) We need to introduce some method to return
`StructuralEquivalenceContext` in ASTImporter. But this is an item for a
separate patch, not this one.
Comment at: lib/AST/ASTImporter.cpp:1890
TypedefNameDecl *ToTypedef;
- if (IsAlias)
-ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC, StartL, Loc,
- Name.getAsIdentifierInfo(), TInfo);
- else
-ToTypedef = TypedefDecl::Create(Importer.getToContext(), DC,
-StartL, Loc,
-Name.getAsIdentifierInfo(),
-TInfo);
+ if (IsAlias && GetImportedOrCreateDecl(
+ ToTypedef, D, Importer.getToContext(), DC, StartL, Loc,
This is not strictly equivalent to the source condition. If
GetImportedOrCreateDecl returns false, we'll fall to the `else` branch, and it
doesn't seem correct to me.
Comment at: lib/AST/ASTImporter.cpp:4274
+ TemplateParams))
+return ToD;
+ return ToD;
Can we just ignore the return value by casting it to void here and in similar
cases?
Comment at: lib/AST/ASTStructuralEquivalence.cpp:895
+ // If any of the records has external storage and we do a minimal check (or
+ // AST import) we assmue they are equivalent. (If we didn't have this
+ // assumption then `RecordDecl::LoadFieldsFromExternalStorage` could trigger
assume
Repository:
rC Clang
https://reviews.llvm.org/D47632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48628: [AST] Structural equivalence of methods
a_sidorin added inline comments.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:489
+
+TEST_F(StructuralEquivalenceRecordTest, DISABLED_Methods) {
+ auto t = makeNamedDecls(
balazske wrote:
> a_sidorin wrote:
> > Could you add a comment why this test is disabled?
> Methods are not checked, there was no decision about to include this check or
> not. The problem was related to performance overhead and if order-independent
> check of methods is needed. (ASTImporter should keep order of imported fields
> and methods.) (This test is about equivalence of `foo`.)
You mean that imported decl have other order of methods? Do you mean implicit
methods (because I see only a single method here)? If so, could you please note
this in the comment?
Repository:
rC Clang
https://reviews.llvm.org/D48628
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48941: [ASTImporter] import FunctionDecl end locations
a_sidorin accepted this revision. a_sidorin added a comment. LGTM too. Thank you! Repository: rC Clang https://reviews.llvm.org/D48941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r336518 - [clang-format/ObjC] Fix counting selector name parts for ObjC
Author: jolesiak
Date: Sun Jul 8 22:58:51 2018
New Revision: 336518
URL: http://llvm.org/viewvc/llvm-project?rev=336518&view=rev
Log:
[clang-format/ObjC] Fix counting selector name parts for ObjC
Summary:
Counts selector parts also for method declarations and counts correctly for
methods without arguments.
This is an internal change and doesn't influence formatting on its own (at the
current state). Its lack would be visible after applying D48719.
Reviewers: benhamilton, klimek
Reviewed By: benhamilton
Subscribers: acoomans, cfe-commits
Differential Revision: https://reviews.llvm.org/D48716
Modified:
cfe/trunk/lib/Format/FormatToken.h
cfe/trunk/lib/Format/TokenAnnotator.cpp
Modified: cfe/trunk/lib/Format/FormatToken.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/FormatToken.h?rev=336518&r1=336517&r2=336518&view=diff
==
--- cfe/trunk/lib/Format/FormatToken.h (original)
+++ cfe/trunk/lib/Format/FormatToken.h Sun Jul 8 22:58:51 2018
@@ -243,8 +243,9 @@ struct FormatToken {
/// e.g. because several of them are block-type.
unsigned LongestObjCSelectorName = 0;
- /// How many parts ObjC selector have (i.e. how many parameters method
- /// has).
+ /// If this is the first ObjC selector name in an ObjC method
+ /// definition or call, this contains the number of parts that the whole
+ /// selector consist of.
unsigned ObjCSelectorNameParts = 0;
/// Stores the number of required fake parentheses and the
Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=336518&r1=336517&r2=336518&view=diff
==
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Sun Jul 8 22:58:51 2018
@@ -515,11 +515,23 @@ private:
}
Left->MatchingParen = CurrentToken;
CurrentToken->MatchingParen = Left;
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when the method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the method call
is
+// reached, in case it was not set already.
+if (!Contexts.back().FirstObjCSelectorName) {
+FormatToken* Previous = CurrentToken->getPreviousNonComment();
+if (Previous && Previous->is(TT_SelectorName)) {
+ Previous->ObjCSelectorNameParts = 1;
+ Contexts.back().FirstObjCSelectorName = Previous;
+}
+} else {
+ Left->ParameterCount =
+ Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
+}
if (Contexts.back().FirstObjCSelectorName) {
Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName =
Contexts.back().LongestObjCSelectorName;
- Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts =
- Left->ParameterCount;
if (Left->BlockParameterCount > 1)
Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName = 0;
}
@@ -539,11 +551,6 @@ private:
TT_DesignatedInitializerLSquare)) {
Left->Type = TT_ObjCMethodExpr;
StartsObjCMethodExpr = true;
- // ParameterCount might have been set to 1 before expression was
- // recognized as ObjCMethodExpr (as '1 + number of commas' formula is
- // used for other expression types). Parameter counter has to be,
- // therefore, reset to 0.
- Left->ParameterCount = 0;
Contexts.back().ColonIsObjCMethodExpr = true;
if (Parent && Parent->is(tok::r_paren))
// FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
@@ -617,12 +624,12 @@ private:
}
void updateParameterCount(FormatToken *Left, FormatToken *Current) {
+// For ObjC methods, the number of parameters is calculated differently as
+// method declarations have a different structure (the parameters are not
+// inside a bracket scope).
if (Current->is(tok::l_brace) && Current->BlockKind == BK_Block)
++Left->BlockParameterCount;
-if (Left->Type == TT_ObjCMethodExpr) {
- if (Current->is(tok::colon))
-++Left->ParameterCount;
-} else if (Current->is(tok::comma)) {
+if (Current->is(tok::comma)) {
++Left->ParameterCount;
if (!Left->Role)
Left->Role.reset(new CommaSeparatedList(Style));
@@ -718,6 +725,7 @@ private:
Contexts.back().LongestObjCSelectorName)
Contexts.back().LongestObjCSelectorName =
Tok->Previous->ColumnWidth;
+ ++Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
}
} else if (Contexts.back().ColonIsForRangeExpr) {
Tok->Type = TT_RangeBasedF
[PATCH] D48716: [clang-format/ObjC] Fix counting selector name parts for ObjC
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336518: [clang-format/ObjC] Fix counting selector name parts
for ObjC (authored by jolesiak, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D48716
Files:
cfe/trunk/lib/Format/FormatToken.h
cfe/trunk/lib/Format/TokenAnnotator.cpp
Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -515,11 +515,23 @@
}
Left->MatchingParen = CurrentToken;
CurrentToken->MatchingParen = Left;
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when the method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the method call
is
+// reached, in case it was not set already.
+if (!Contexts.back().FirstObjCSelectorName) {
+FormatToken* Previous = CurrentToken->getPreviousNonComment();
+if (Previous && Previous->is(TT_SelectorName)) {
+ Previous->ObjCSelectorNameParts = 1;
+ Contexts.back().FirstObjCSelectorName = Previous;
+}
+} else {
+ Left->ParameterCount =
+ Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
+}
if (Contexts.back().FirstObjCSelectorName) {
Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName =
Contexts.back().LongestObjCSelectorName;
- Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts =
- Left->ParameterCount;
if (Left->BlockParameterCount > 1)
Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName = 0;
}
@@ -539,11 +551,6 @@
TT_DesignatedInitializerLSquare)) {
Left->Type = TT_ObjCMethodExpr;
StartsObjCMethodExpr = true;
- // ParameterCount might have been set to 1 before expression was
- // recognized as ObjCMethodExpr (as '1 + number of commas' formula is
- // used for other expression types). Parameter counter has to be,
- // therefore, reset to 0.
- Left->ParameterCount = 0;
Contexts.back().ColonIsObjCMethodExpr = true;
if (Parent && Parent->is(tok::r_paren))
// FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
@@ -617,12 +624,12 @@
}
void updateParameterCount(FormatToken *Left, FormatToken *Current) {
+// For ObjC methods, the number of parameters is calculated differently as
+// method declarations have a different structure (the parameters are not
+// inside a bracket scope).
if (Current->is(tok::l_brace) && Current->BlockKind == BK_Block)
++Left->BlockParameterCount;
-if (Left->Type == TT_ObjCMethodExpr) {
- if (Current->is(tok::colon))
-++Left->ParameterCount;
-} else if (Current->is(tok::comma)) {
+if (Current->is(tok::comma)) {
++Left->ParameterCount;
if (!Left->Role)
Left->Role.reset(new CommaSeparatedList(Style));
@@ -718,6 +725,7 @@
Contexts.back().LongestObjCSelectorName)
Contexts.back().LongestObjCSelectorName =
Tok->Previous->ColumnWidth;
+ ++Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
}
} else if (Contexts.back().ColonIsForRangeExpr) {
Tok->Type = TT_RangeBasedForLoopColon;
Index: cfe/trunk/lib/Format/FormatToken.h
===
--- cfe/trunk/lib/Format/FormatToken.h
+++ cfe/trunk/lib/Format/FormatToken.h
@@ -243,8 +243,9 @@
/// e.g. because several of them are block-type.
unsigned LongestObjCSelectorName = 0;
- /// How many parts ObjC selector have (i.e. how many parameters method
- /// has).
+ /// If this is the first ObjC selector name in an ObjC method
+ /// definition or call, this contains the number of parts that the whole
+ /// selector consist of.
unsigned ObjCSelectorNameParts = 0;
/// Stores the number of required fake parentheses and the
Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -515,11 +515,23 @@
}
Left->MatchingParen = CurrentToken;
CurrentToken->MatchingParen = Left;
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when the method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the method call is
+// reached, in case it was not set already.
+if (!Contexts.back().FirstObjCSelectorName) {
+FormatToken* Previous = Curr
r336519 - [clang-format/ObjC] Prohibit breaking after a bracket opening ObjC method expression
Author: jolesiak Date: Sun Jul 8 23:04:58 2018 New Revision: 336519 URL: http://llvm.org/viewvc/llvm-project?rev=336519&view=rev Log: [clang-format/ObjC] Prohibit breaking after a bracket opening ObjC method expression Summary: Don't break after a "[" opening an ObjC method expression. Tests are added in D48719 where formatting is improved (to avoid adding and changing tests immediately). Reviewers: benhamilton, klimek Reviewed By: benhamilton Subscribers: acoomans, cfe-commits Differential Revision: https://reviews.llvm.org/D48718 Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=336519&r1=336518&r2=336519&view=diff == --- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original) +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Sun Jul 8 23:04:58 2018 @@ -321,6 +321,9 @@ bool ContinuationIndenter::canBreak(cons State.Stack.back().NoLineBreakInOperand) return false; + if (Previous.is(tok::l_square) && Previous.is(TT_ObjCMethodExpr)) +return false; + return !State.Stack.back().NoLineBreak; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48718: [clang-format/ObjC] Prohibit breaking after a bracket opening ObjC method expression
This revision was automatically updated to reflect the committed changes. Closed by commit rL336519: [clang-format/ObjC] Prohibit breaking after a bracket opening ObjC method… (authored by jolesiak, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D48718 Files: cfe/trunk/lib/Format/ContinuationIndenter.cpp Index: cfe/trunk/lib/Format/ContinuationIndenter.cpp === --- cfe/trunk/lib/Format/ContinuationIndenter.cpp +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp @@ -321,6 +321,9 @@ State.Stack.back().NoLineBreakInOperand) return false; + if (Previous.is(tok::l_square) && Previous.is(TT_ObjCMethodExpr)) +return false; + return !State.Stack.back().NoLineBreak; } Index: cfe/trunk/lib/Format/ContinuationIndenter.cpp === --- cfe/trunk/lib/Format/ContinuationIndenter.cpp +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp @@ -321,6 +321,9 @@ State.Stack.back().NoLineBreakInOperand) return false; + if (Previous.is(tok::l_square) && Previous.is(TT_ObjCMethodExpr)) +return false; + return !State.Stack.back().NoLineBreak; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48628: [AST] Structural equivalence of methods
balazske marked an inline comment as done.
balazske added inline comments.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:489
+
+TEST_F(StructuralEquivalenceRecordTest, DISABLED_Methods) {
+ auto t = makeNamedDecls(
a_sidorin wrote:
> balazske wrote:
> > a_sidorin wrote:
> > > Could you add a comment why this test is disabled?
> > Methods are not checked, there was no decision about to include this check
> > or not. The problem was related to performance overhead and if
> > order-independent check of methods is needed. (ASTImporter should keep
> > order of imported fields and methods.) (This test is about equivalence of
> > `foo`.)
> You mean that imported decl have other order of methods? Do you mean implicit
> methods (because I see only a single method here)? If so, could you please
> note this in the comment?
The problem with the ordering is in general at structural equivalence check. If
a complex structure is imported with ASTImporter the ordering of fields and
methods may change in the imported class (relative to the to-be imported) after
the import. Either order-independent check is needed in structural equivalence
check to match two classes with same methods but in different order, or import
should not change ordering. Currently none of these is implemented, methods are
not checked at all. The `foo`s in this test should be non-equivalent because
one different method `x`.
Repository:
rC Clang
https://reviews.llvm.org/D48628
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
balazske added inline comments. Comment at: lib/AST/ASTImporter.cpp:6905 Decl *ToD = Pos->second; +// FIXME: remove this call from this function ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, ToD); I think this comment is not needed (or with other text). There is a case when `GetAlreadyImportedOrNull` is called during import of a Decl that is already imported but its definition is not yet completely imported. If this call is here we have after `GetAlreadyImportedOrNull` a Decl with complete definition. (Name of the function is still bad: It does not only "get" but makes update too. The `ImportDefinitionIfNeeded` call can be moved into the decl create function?) Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r336520 - [clang-format/ObjC] Improve split priorities for ObjC methods
Author: jolesiak
Date: Sun Jul 8 23:54:52 2018
New Revision: 336520
URL: http://llvm.org/viewvc/llvm-project?rev=336520&view=rev
Log:
[clang-format/ObjC] Improve split priorities for ObjC methods
Reduce penalty for aligning ObjC method arguments using the colon alignment as
this is the canonical way.
Trying to fit a whole expression into one line should not force other line
breaks (e.g. when ObjC method expression is a part of other expression).
Modified:
cfe/trunk/lib/Format/FormatToken.h
cfe/trunk/lib/Format/TokenAnnotator.cpp
cfe/trunk/unittests/Format/FormatTestObjC.cpp
Modified: cfe/trunk/lib/Format/FormatToken.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/FormatToken.h?rev=336520&r1=336519&r2=336520&view=diff
==
--- cfe/trunk/lib/Format/FormatToken.h (original)
+++ cfe/trunk/lib/Format/FormatToken.h Sun Jul 8 23:54:52 2018
@@ -248,6 +248,11 @@ struct FormatToken {
/// selector consist of.
unsigned ObjCSelectorNameParts = 0;
+ /// The 0-based index of the parameter/argument. For ObjC it is set
+ /// for the selector name token.
+ /// For now calculated only for ObjC.
+ unsigned ParameterIndex = 0;
+
/// Stores the number of required fake parentheses and the
/// corresponding operator precedence.
///
Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=336520&r1=336519&r2=336520&view=diff
==
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Sun Jul 8 23:54:52 2018
@@ -725,6 +725,8 @@ private:
Contexts.back().LongestObjCSelectorName)
Contexts.back().LongestObjCSelectorName =
Tok->Previous->ColumnWidth;
+ Tok->Previous->ParameterIndex =
+ Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
++Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
}
} else if (Contexts.back().ColonIsForRangeExpr) {
@@ -2142,8 +2144,20 @@ void TokenAnnotator::calculateFormatting
// FIXME: Only calculate this if CanBreakBefore is true once static
// initializers etc. are sorted out.
// FIXME: Move magic numbers to a better place.
-Current->SplitPenalty = 20 * Current->BindingStrength +
-splitPenalty(Line, *Current, InFunctionDecl);
+
+// Reduce penalty for aligning ObjC method arguments using the colon
+// alignment as this is the canonical way (still prefer fitting everything
+// into one line if possible). Trying to fit a whole expression into one
+// line should not force other line breaks (e.g. when ObjC method
+// expression is a part of other expression).
+Current->SplitPenalty = splitPenalty(Line, *Current, InFunctionDecl);
+if (Style.Language == FormatStyle::LK_ObjC &&
+Current->is(TT_SelectorName) && Current->ParameterIndex > 0) {
+ if (Current->ParameterIndex == 1)
+Current->SplitPenalty += 5 * Current->BindingStrength;
+} else {
+ Current->SplitPenalty += 20 * Current->BindingStrength;
+}
Current = Current->Next;
}
Modified: cfe/trunk/unittests/Format/FormatTestObjC.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestObjC.cpp?rev=336520&r1=336519&r2=336520&view=diff
==
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp Sun Jul 8 23:54:52 2018
@@ -678,6 +678,18 @@ TEST_F(FormatTestObjC, FormatObjCMethodE
verifyFormat("[(id)foo bar:(id) ? baz : quux];");
verifyFormat("4 > 4 ? (id)a : (id)baz;");
+ unsigned PreviousColumnLimit = Style.ColumnLimit;
+ Style.ColumnLimit = 50;
+ // Instead of:
+ // bool a =
+ // ([object a:42] == 0 || [object a:42
+ //b:42] == 0);
+ verifyFormat("bool a = ([object a:42] == 0 ||\n"
+ " [object a:42 b:42] == 0);");
+ Style.ColumnLimit = PreviousColumnLimit;
+ verifyFormat("bool a = ([ a] == a ||\n"
+ " [ a] == );");
+
// This tests that the formatter doesn't break after "backing" but before
":",
// which would be at 80 columns.
verifyFormat(
@@ -754,11 +766,10 @@ TEST_F(FormatTestObjC, FormatObjCMethodE
"[self a:aaa, aaa,
aaa,\n"
"aaa, aaa,
aaa,\n"
"aaa, aaa];");
+
verifyFormat("[self // break\n"
" a:a\n"
"aaa:aaa];");
- verifyFormat("bool a = ([ a] == a
