zaks.anna added inline comments.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:67
@@ +66,3 @@
+  // Every time a request is 'set' a new 'RequestId' gets created.
+  // Therefore, the 'UserKind' does not need to be profiled.
+  const int RequestId{RequestIdCount++};
----------------
Alexander_Droste wrote:
> zaks.anna wrote:
> > Still it looks like you have several pieces of information in the state 
> > that are redundant.. Looks like you've added more now.
> > 
> > For example, why do we need RequestId? Each report will only talk about a 
> > single request, is this not the case?
> > 
> > Do you absolutely need to store LastUser and VariableName?
> > 
> > Note that storing in the state is very expensive; on the other hand, we can 
> > afford to perform costly analysis on post-processing of an error report. 
> > This is why all other checkers store minimal information in the state, 
> > usually just a single enum and determine all the other information during 
> > the walk over the error path in BugReporterVisitor. The visitor will visit 
> > all nodes that you visited while reporting this bug, so it should have 
> > access to all the information you need.
> > 
> I need the RequestID to distinct function calls of the same type 
> and location using the request multiple times along the path.
> Like in a loop:
> ```
> for int i ...
>    nonblocking(.., request) // double nonblocking
> ```
> 
> > Do you absolutely need to store LastUser and VariableName?
> Absolutely not. :D I will remove the string member.
> The variable name retrieval can be delayed to the point where the report is 
> created.
> 
> I can also remove the enum, as the state of the request can be derived from 
> the LastUser.
> The reason why I added the enum member, is that I was concerned about that 
> the CallEventRef
> can be invalid to reference again if the call is inlined from an 
> implementation defined in a header. 
> As this seems not to be the case, I can remove the redundancy.
> 
> > This is why all other checkers store minimal information in the state, 
> > usually just a single enum and determine all the other information during 
> > the walk over the error path in BugReporterVisitor.
> 
> Ah, I get it. Until now that seemed a bit overly complicated to me.
I suspect the only thing you need in Request is the enum; everything else can 
be determined while visiting the path. 

This should be in ento namespace not in the clang namespace.


http://reviews.llvm.org/D12761



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to