[PATCH] D44524: Add an analysis printer for must execute reasoning

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: lib/Analysis/MustExecute.cpp:87
+    OS << "\t(mustexec in ";
+    bool first = true;
+    for (const Loop *L : MustExec.lookup(V)) {
----------------
anna wrote:
> anna wrote:
> > reames wrote:
> > > anna wrote:
> > > > reames wrote:
> > > > > anna wrote:
> > > > > > Perhaps rename this to exactlyOneLoop and  define that as  `MustExec[V]->size()  == 1`  
> > > > > Er, what?  I don't understand your comment.  The "first" variable is simply being used to join a set of strings with a comma in between.  
> > > > What I meant is we can do something like this instead of using the first variable:
> > > > ```
> > > > const bool exactlyOneLoop = MustExec[V]->size()  == 1;
> > > > for (const Loop *L : MustExec.lookup(V)) {
> > > >       OS << L->getHeader()->getName();
> > > >       if (!ExactlyOneLoop)
> > > >           OS << ", ";
> > > > }
> > > > ```
> > > > 
> > > Ah, I see where you're going.  And actually you're pointing out a bug in the code.  What I'd actually wanted was: "name, name, name".  What I'd wrote produced "name name, name, "  Your suggestion is a cleanup of the code as written, but doesn't work once I fix the formatting bug. 
> > I didn't notice the bug originally, but I think the cleanup should fix the bug :)
> > We should produce "name, name, name" because the boolean `exactlyOneLoop` is a constant and set outside the loop. We don't change it (unlike the `first` variable which was changed in the loop, and was the reason for the bug).
> nvm.. just took a look at it again. It fixes one part of the formatting: one comma after the first name. However, there's one extra comma after the last one. 
Nope, your variant produces: "name, name, ".  (With the trailing ", ")


Repository:
  rL LLVM

https://reviews.llvm.org/D44524