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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
