[PATCH] [patch] Adding Consumed Analysis to Clang
David Blaikie
dblaikie at gmail.com
Tue Jul 30 17:54:14 PDT 2013
Here's a few ideas, mostly micro details. I haven't quite got the macro design sufficiently in my head to respond to that yet.
================
Comment at: test/SemaCXX/warn-consumed-analysis.cpp:46
@@ +45,3 @@
+ if (var.isValid()) { // \
+ \\ expected-warning {{Unnecessary test. Variable 'var' is known to be in the 'consumed' state}}
+ *var;
----------------
Line wrapping here is strange (comment, newline escape, then another comment starter?). Generally we don't worry about 80 cols in tests, so you can just remove the line wrapping.
================
Comment at: include/clang/Analysis/Analyses/Consumed.h:39
@@ +38,3 @@
+ // No state information for the given variable.
+ None,
+
----------------
See the naming conventions for enumerators here: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
================
Comment at: include/clang/Analysis/Analyses/Consumed.h:56
@@ +55,3 @@
+
+ typedef llvm::DenseMap<const VarDecl*, ConsumedState> MapType;
+ typedef std::pair<const VarDecl*, ConsumedState> PairType;
----------------
Might want to run clang-format over the code, it'll add things like the space for * here (to become "const VarDecl *")
================
Comment at: include/clang/Analysis/Analyses/Consumed.h:62
@@ +61,3 @@
+ //////////////////
+ // Data Members //
+ //////////////////
----------------
Drop the section headers like this - they're not really done in LLVM code so far as I know.
================
Comment at: include/clang/Analysis/Analyses/Consumed.h:73
@@ +72,3 @@
+ ConsumedStateMap(void) {}
+ ConsumedStateMap(ConsumedStateMap &Other) : Map(Other.Map) {}
+
----------------
I assume this should be a const reference parameter (non-const copy constructors are weird)
Better than that - can you just not define either constructor & rely on the defaults? That way you should hopefully get a move ctor for free. (OK, granted DenseMap would have to be movable for that - but that's someone else's problem unless you want to make it yours)
================
Comment at: include/clang/Analysis/Analyses/Consumed.h:72
@@ +71,3 @@
+ public:
+ ConsumedStateMap(void) {}
+ ConsumedStateMap(ConsumedStateMap &Other) : Map(Other.Map) {}
----------------
Drop 'void' from the parameter list. This isn't necessary in C++ & not prevalent in the LLVM codebase. (see comment below about removing unnecessarily explicit ctor implementations)
================
Comment at: include/clang/Analysis/Analyses/Consumed.h:79
@@ +78,3 @@
+ /// \brief Mark all variables as unknown.
+ void makeUnknown(void);
+
----------------
Another 'void' in the param list. Consider this general feedback & I won't call out any other instances I see.
================
Comment at: lib/Analysis/Consumed.cpp:402
@@ +401,3 @@
+
+void ConsumedStateMap::merge(const ConsumedStateMap *Other) {
+ ConsumedState LocalState;
----------------
Looks like 'Other' is unconditionally dereferenced and thus cannot be null - perhaps it should be be passed by const reference instead of pointer?
================
Comment at: lib/Analysis/Consumed.cpp:405
@@ +404,3 @@
+
+ for (MapType::const_iterator DMI = Other->Map.begin(),
+ DME = Other->Map.end(); DMI != DME; ++DMI) {
----------------
For a short scope iterator, consider just calling it 'I'
================
Comment at: lib/Analysis/Consumed.cpp:408
@@ +407,3 @@
+
+ PairType OtherPair = *DMI;
+
----------------
Not strictly unacceptable, but consider not copying this local and just using I->first or I->second. For a short loop I personally find it easier to read as I can quickly spot the "this is the current element" code" when it directly uses the iterator. Open to disagreement/other ideas, though.
================
Comment at: lib/Analysis/Consumed.cpp:414
@@ +413,3 @@
+ Map.insert(OtherPair);
+
+ } else if (LocalState != OtherPair.second) {
----------------
Unnecessary blank line
================
Comment at: lib/Analysis/Consumed.cpp:412
@@ +411,3 @@
+
+ if (LocalState == None) {
+ Map.insert(OtherPair);
----------------
Unnecessary { on one expression block
================
Comment at: lib/Analysis/Consumed.cpp:416
@@ +415,3 @@
+ } else if (LocalState != OtherPair.second) {
+ Map.erase(OtherPair.first);
+ Map.insert(PairType(OtherPair.first, Unknown));
----------------
Could make this one line (& drop the braces) by calling setState?
================
Comment at: lib/Analysis/Consumed.cpp:443
@@ +442,3 @@
+
+ const OptionalNotes &Notes = I->second;
+ S.Diag(I->first.first, I->first.second);
----------------
I'd consider moving this declaration down to just before the loop so it's clearer what it's for (& since it's not needed for the intervening line of code - so this would adhere to the general principle of least scope)
================
Comment at: lib/Analysis/Consumed.cpp:455
@@ +454,3 @@
+bool ConsumedAnalyzer::isConsumableType(QualType Type) {
+ if (const CXXRecordDecl *Klass =
+ dyn_cast_or_null<CXXRecordDecl>(Type->getAsCXXRecordDecl())) {
----------------
It'd be more idiomatic to name this variable "RD".
================
Comment at: lib/Analysis/Consumed.cpp:461
@@ +460,3 @@
+ if (Entry != ConsumableTypeCache.end()) {
+ return (*Entry).second;
+ }
----------------
-> rather than (*x).y
================
Comment at: lib/Analysis/Consumed.cpp:461
@@ +460,3 @@
+ if (Entry != ConsumableTypeCache.end()) {
+ return (*Entry).second;
+ }
----------------
David Blaikie wrote:
> -> rather than (*x).y
remove {} from one line block
================
Comment at: lib/Analysis/Consumed.cpp:458
@@ +457,3 @@
+
+ CacheMapType::const_iterator Entry = ConsumableTypeCache.find(Klass);
+
----------------
Microoptimization:
Rather than find+insert, if you use insert here (with a default value for the boolean 'value'), test the second element of the pair returned and then populate the value if it was newly inserted. Something like this might be nice:
std::pair<CacheMapType::iterator, bool> Entry = ConsumableTypeCache.insert(std::make_pair(RD, false));
if (Entry.second)
Entry.first->second = hasConsumableAttributes(*RD);
return Entry.first->second;
(where the loop over the CXXRecordDecl's methods and attributes is pulled out into a 'hasConsumableAttributes' function)
================
Comment at: lib/Analysis/Consumed.cpp:488
@@ +487,3 @@
+
+// FIXME: Make this not generate false positives for while- and for-loops.
+// TODO: Handle other forms of branching with precision, including while- and
----------------
Is this fixme still valid? I remember you spoke to me about this issue & thought you'd ended up punting on this & classifying loop blocks as causing an "unspecified" state.
================
Comment at: lib/Analysis/Consumed.cpp:455
@@ +454,3 @@
+bool ConsumedAnalyzer::isConsumableType(QualType Type) {
+ if (const CXXRecordDecl *Klass =
+ dyn_cast_or_null<CXXRecordDecl>(Type->getAsCXXRecordDecl())) {
----------------
David Blaikie wrote:
> It'd be more idiomatic to name this variable "RD".
There's a function further down ( ConsumedAnalyzer::run) that starts with a dyn_cast_or_null and then an "if (!D) return" - this seems tidier than nesting the whole body of the function. Consider doing the same here.
================
Comment at: lib/Analysis/Consumed.cpp:530
@@ +529,3 @@
+
+ if (!D) {
+ return;
----------------
Unnecessary {}. Once you remove those, clang-format should choose whether to put the return on the same line as the 'if' or a separate line. I forget which way it goes.
================
Comment at: lib/Analysis/Consumed.cpp:523
@@ +522,3 @@
+
+ BlockInfo.addInfo( *SI, CurrStates);
+ BlockInfo.addInfo(*++SI, ElseOrMergeStates);
----------------
This indentation isn't really common in LLVM - clang-format should choose appropriately.
================
Comment at: lib/Analysis/Consumed.cpp:513
@@ +512,3 @@
+ if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var, Consumed);
+
+ } else {
----------------
Trailing blank line in a block seems strange (& there's a few more blank lines than I'd expect here - could be wrong though. Again, whatever clang-format does, I'm OK with (though I'm not sure it touches blank lines, in which case I'll express some preferences))
================
Comment at: lib/Analysis/Consumed.cpp:565
@@ +564,3 @@
+
+ // TODO: Remove any variables that have reached the end of their
+ // lifetimes from the state map. (Deferred)
----------------
Any idea how bad this'll hurt the compiler for large functions? I assume we only track variables with these annotations - so we're not going to kill the compiler tomorrow since nothing is annotated, right?
================
Comment at: lib/Analysis/Consumed.cpp:574
@@ +573,3 @@
+
+ } else if (CurrBlock->succ_size() == 2) {
+ // Handle while- and for-loops.
----------------
Should this case just be moved to the end of this if/else if chain and just be an unconditional 'else'?
(& what happens if there are more than 2 successors? (switch block?))
================
Comment at: lib/Analysis/Consumed.cpp:595
@@ +594,3 @@
+ // Delete the last existing state map.
+ delete CurrStates;
+
----------------
An owning smart pointer would be nice - perhaps CurrStates should be an OwningPtr<T>?
Will this lead to a double-delete when you cleanup the contents of BlockInfo (hmm, don't seem to be cleaning those up at all)? I'd like a bit more clear ownership (perhaps keeping the ConsumedStateMaps directly in the BlockInfo, rather than pointers)
================
Comment at: lib/Analysis/Consumed.cpp:641
@@ +640,3 @@
+bool isTestingFunction(const CXXMethodDecl *Method) {
+ if (Method->hasAttr<TestsUnconsumedAttr>()) {
+ return true;
----------------
just "return Method->hasAttr<TestsUnconsumedAttr>();" instead? Or perhaps skip the function and make this call directly at the callsites if it's sufficiently legible.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1017
@@ +1016,3 @@
+
+ if (not isa<CXXMethodDecl>(D)) {
+ S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<
----------------
not -> !
(& several more)
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1030
@@ +1029,3 @@
+ const AttributeList &Attr) {
+ // Handle functions with the CALLABLE_ALWAYS attribute.
+
----------------
Comment (& the one up at 1009) seem a bit unnecessary. The function name is self explanatory.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1076
@@ +1075,3 @@
+
+ if (not checkAttributeNumArgs(S, Attr, 0)) {
+ return;
----------------
More one-statement blocks with {}
================
Comment at: test/SemaCXX/warn-consumed-analysis.cpp:33
@@ +32,3 @@
+ if (var0.isValid()) { // \
+ \\ expected-warning {{Unnecessary test. Variable 'var0' is known to be in the 'consumed' state}}
+ *var0;
----------------
I think we should drop the "unnecessary test" warning for now. Feel free to leave a comment where it would go (probably not even to the level of FIXME, just "A warning could be emitted here about a constant test, but care should be taken regarding things such as macro expansions & templates.")
================
Comment at: test/SemaCXX/warn-consumed-analysis.cpp:35
@@ +34,3 @@
+ *var0;
+ *var1; // expected-warning {{Invocation of method 'operator*' on object 'var1' while it is in the 'consumed' state}}
+
----------------
I wonder whether there's a way we could make these diagnostics more legible for the unique_ptr case. Could we have some verbs in the attributes? (I guess at that point we'd need a class level attribute for a consumable type - which would make it easier to decide whether a type was consumable (save you the member search). Is there any other way in which we'd benefit from/use a class level attribute?)
================
Comment at: test/SemaCXX/warn-consumed-analysis.cpp:73
@@ +72,3 @@
+
+ *var; // expected-warning {{Invocation of method 'operator*' on object 'var' while it is in an unknown state}}
+}
----------------
As we discussed - unknown state objects should allow all operations (operations allowable only on valid or only on invalid objects)
================
Comment at: test/SemaCXX/warn-consumed-parsing.cpp:10
@@ +9,3 @@
+ void Consumes(void) __attribute__ ((consumes(42))); // \
+ // expected-error {{attribute takes no arguments}}
+ bool TestsUnconsumed(void) __attribute__ ((tests_unconsumed(42))); // \
----------------
Weird line wrapping again. If you'd really like to put these on separate lines you can use the @-1 notation (check other tests for examples - I think that's the syntax) to refer to lines by offset, rather than the current line.
If you're going to do that, I'd consider putting the comment above the line, rather than below.
================
Comment at: lib/Analysis/Consumed.cpp:52
@@ +51,3 @@
+
+//////////////////////
+// Helper Functions //
----------------
more big headers we don't tend to write - file-local functions are basically by definition 'helper functions'.
================
Comment at: lib/Analysis/Consumed.cpp:60
@@ +59,3 @@
+ return "none";
+
+ case consumed::Unknown:
----------------
Clang-format should tidy this up, probably dropping the empty lines & maybe one-lining the case+returns (& perhaps outdenting the cases to line up with the switch - at least I think that's LLVM's preferred style)
================
Comment at: lib/Analysis/Consumed.cpp:103
@@ +102,3 @@
+ ConsumedStateMap *StateMap) : Analyzer(Analyzer), StateMap(StateMap) {
+ ModeStack.push_back(Disabled);
+ }
----------------
Could do this in the ctor init list with "ModeStack(1, Disabled)" but I don't know if that's much/any better.
Or you could call "reset" to keep the functionality in one place.
================
Comment at: lib/Analysis/Consumed.cpp:149
@@ +148,3 @@
+ unsigned NumArgParamPairs =
+ std::min(Call->getNumArgs(), FunDecl->getNumParams());
+
----------------
Could this ever be less than Call->getNumArgs? (perhaps with variadic functions?)
================
Comment at: lib/Analysis/Consumed.cpp:190
@@ +189,3 @@
+
+ if (const FunctionDecl *FunDecl =
+ dyn_cast_or_null<FunctionDecl>(Call->getDirectCallee())) {
----------------
consider having a short return with an inverted condition here, then outdenting the body of this instead of having it all inside the 'if'
================
Comment at: lib/Analysis/Consumed.cpp:195
@@ +194,3 @@
+ // have to check for the different attributes and change the behavior
+ // bellow. (Deferred)
+ if (!FunDecl->hasAttr<CallableWhenUnconsumedAttr>()) return false;
----------------
FIXMEs are by definition deferred, so you don't really need "(Deferred)".
================
Comment at: lib/Analysis/Consumed.cpp:199
@@ +198,3 @@
+ if (const DeclRefExpr *DeclRef =
+ dyn_cast_or_null<DeclRefExpr>(Call->getArg(0))) {
+
----------------
Another possible early return.
================
Comment at: lib/Analysis/Consumed.cpp:201
@@ +200,3 @@
+
+ if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(DeclRef->getDecl())) {
+ switch (StateMap->getState(Var)) {
----------------
& a 3rd
================
Comment at: lib/Analysis/Consumed.cpp:361
@@ +360,3 @@
+ // Merge the new state map with the existing one, and then free the new map.
+ (*Entry).second->merge(StateMap);
+ delete StateMap;
----------------
(*x).y -> x->y
================
Comment at: lib/Analysis/Consumed.cpp:367
@@ +366,3 @@
+ConsumedStateMap* ConsumedBlockInfo::getInfo(const CFGBlock *Block) {
+ ConsumedStateMap *RetValue = (*Map.find(Block)).second;
+
----------------
(*x).y -> x->y
================
Comment at: lib/Analysis/Consumed.cpp:382
@@ +381,3 @@
+ if (Entry != Map.end()) {
+ return (*Entry).second;
+
----------------
(*x).y -> x->y
================
Comment at: lib/Analysis/Consumed.cpp:384
@@ +383,3 @@
+
+ } else {
+ return None;
----------------
Else after return (just write it as "if (x) return; return;" rather than "if (x) return; else return;" )
================
Comment at: lib/Analysis/Consumed.cpp:397
@@ +396,3 @@
+
+ Map.erase(Pair.first);
+ Map.insert(PairType(Pair.first, Unknown));
----------------
Do you need to erase/insert just to change the value? You should just be able to do it with "I->second = Unknown";
(oh, you'd need the iterators to be non-const iterators)
================
Comment at: lib/Analysis/Consumed.cpp:425
@@ +424,3 @@
+void ConsumedStateMap::setState(const VarDecl *Var, ConsumedState State) {
+ Map.erase(Var);
+ Map.insert(PairType(Var, State));
----------------
Map[Var] = State; should suffice for this function
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2175
@@ +2174,3 @@
+def warn_use_while_consumed : Warning<
+ "Invocation of method '%0' on object '%1' while it is in the 'consumed' "
+ "state">,
----------------
Notice that other diagnostics don't begin with an uppercase - they're sentence fragments, not sentences. Rephrase these diagnostics in a similar manner.
If possible, it'd be nice not to talk about "states" explicitly. Perhaps:
"attempt to use '%0' on a consumed object '%1'" ? Open to other wordsmithing...
http://llvm-reviews.chandlerc.com/D1233
More information about the cfe-commits
mailing list