[cfe-dev] Visit nodes in the order of the user output to create better reports

[Csaba Dabis via cfe-dev]

Thanks you! This is a very good idea. I just wanted to rewrite the root of
the visiting to prevent problems like that in the future, as the following
(in BugReporter.cpp):

static std::unique_ptr<VisitorsDiagnosticsTy>
generateVisitorsDiagnostics(BugReport *R, const ExplodedNode *ErrorNode,
                            BugReporterContext &BRC) {
  typedef std::vector<const ExplodedNode *> NodePath;

  auto Notes = llvm::make_unique<VisitorsDiagnosticsTy>();
  BugReport::VisitorList visitors;
  NodePath Path;

  // Run visitors on all nodes starting from the node *before* the last one.
  // The last node is reserved for notes generated with {@code getEndPath}.
  const ExplodedNode *Pred = ErrorNode;
  while (Pred) {
    Pred = Pred->getFirstPred();
    if (!Pred)
      break;

    if (!R->isValid())
      break;

    Path.push_back(Pred);
  }

  for (NodePath::reverse_iterator I = Path.rbegin(); I != Path.rend(); ++I)
{
    const ExplodedNode *NextNode = *I;

    // At each iteration, move all visitors from report to visitor list.
    for (BugReport::visitor_iterator VI = R->visitor_begin(),
                                     E = R->visitor_end();
         VI != E; ++VI) {
      visitors.push_back(std::move(*VI));
    }
    R->clearVisitors();

    for (auto &V : visitors) {
      llvm::errs() << "\nbefore crash - it is written out";
      auto P = V->VisitNode(NextNode, BRC, *R);
      llvm::errs() << "\nafter crash";
      if (P)
        (*Notes)[NextNode].push_back(std::move(P));
    }

    if (!R->isValid())
      break;
  }

  std::shared_ptr<PathDiagnosticPiece> LastPiece;
  for (auto &V : visitors) {
    V->finalizeVisitor(BRC, ErrorNode, *R);

    if (auto Piece = V->getEndPath(BRC, ErrorNode, *R)) {
      assert(!LastPiece &&
             "There can only be one final piece in a diagnostic.");
      LastPiece = std::move(Piece);
      (*Notes)[ErrorNode].push_back(LastPiece);
    }
  }

  return Notes;
}

I think you are right about it I should change things locally, so I will
drop that patch, because it is too difficult for me.

George Karpenkov <ekarpenkov at apple.com> ezt írta (időpont: 2018. nov. 16.,
P, 19:25):

> I’m still not exactly sure what are you doing,
> but you can store all the information you need in a vector inside the
> visitor,
> and then iterate over it in the callback for the last node.
> Is that what you’ve tried originally? Where does it crash?
>
> On Nov 16, 2018, at 10:22 AM, Csaba Dabis <dabis.csaba98 at gmail.com> wrote:
>
> The current way of bug reporting:
> ---
> 1 Assuming 'i' is not equal to 1
> ----
> 2 Assuming 'i' is not equal to 2
> 3 Taking false branch
> 4 Taking false branch
> ---
>
> If I would like to hook extra information in the linear backwards way,
> this is going to be like the following:
> ---
> 1 Assuming 'i' is not equal to 1
> ----
> 2 Knowing 'i' is not equal to 2
> 3 Knowing 'i' is not equal to 1
> 4 Assuming 'i' is not equal to [1, 2]
> ---
>
> And actually the proper approach would be:
> ---
> 1 Assuming 'i' is not equal to 1
> ---
> 2 Assuming 'i' is not equal to 2
> 3 Knowing 'i' is not equal to [1, 2]
> 4 Knowing 'i' is not equal to [1, 2]
> ---
>
> George Karpenkov <ekarpenkov at apple.com> ezt írta (időpont: 2018. nov.
> 16., P, 19:04):
>
>> Going forward is ambiguous, as there are many leafs, but only one root.
>> What are you trying to achieve? Going backwards should never be a problem.
>>
>> > On Nov 16, 2018, at 8:21 AM, Csaba Dabis via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>> >
>> > Hello!
>> >
>> > Why does the 'generateVisitorsDiagnostics()' function in
>> BugReporter.cpp work backwards? It starts from the node of the error and
>> iterate over the preds.
>> >
>> > I have tried to make it to work forward with storing the 'const
>> ExplodedNode *' nodes in a vector, but 'auto P = V->VisitNode(NextNode,
>> BRC, *R);' crashes. I just found out the 'LastPiece' checking could be
>> moved out from the while-loop, that is it for now.
>> >
>> > Is it possible to achieve? What is could be problematic with that code
>> snippet?
>> >
>> > Thanks you for the suggestions,
>> > Csaba.
>> > _______________________________________________
>> > cfe-dev mailing list
>> > cfe-dev at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20181116/3e06b90f/attachment.html>