[llvm-branch-commits] [llvm] [BOLT] Gadget scanner: refactor issue reporting (PR #135662)

[Anatoly Trosinenko via llvm-branch-commits]

================
@@ -263,8 +242,75 @@ struct GenericReport : public Report {
                               const BinaryContext &BC) const override;
 };
 
+/// An information about an issue collected on the slower, detailed,
+/// run of an analysis.
+class ExtraInfo {
+public:
+  virtual void print(raw_ostream &OS, const MCInstReference Location) const = 0;
+
+  virtual ~ExtraInfo() {}
+};
+
+class ClobberingInfo : public ExtraInfo {
+  SmallVector<MCInstReference> ClobberingInstrs;
+
+public:
+  ClobberingInfo(const ArrayRef<MCInstReference> Instrs)
+      : ClobberingInstrs(Instrs) {}
+
+  void print(raw_ostream &OS, const MCInstReference Location) const override;
+};
+
+/// A brief version of a report that can be further augmented with the details.
+///
+/// It is common for a particular type of gadget detector to be tied to some
+/// specific kind of analysis. If an issue is returned by that detector, it may
+/// be further augmented with the detailed info in an analysis-specific way,
+/// or just be left as-is (f.e. if a free-form warning was reported).
+template <typename T> struct BriefReport {
+  BriefReport(std::shared_ptr<Report> Issue,
+              const std::optional<T> RequestedDetails)
+      : Issue(Issue), RequestedDetails(RequestedDetails) {}
----------------
atrosinenko wrote:

> If I understand the code correctly, `Report`, `GadgetReport` and `GenericReport` are similar, but `BriefReport` and `DetailedReport` really are something somewhat different.

Yes, these are two groups of classes with misleading common "report" suffix. I like the idea of replacing `Report` with `Diagnostic` in `Report`, `GadgetReport` and `GenericReport`, thanks! This way, the "report" suffix is left for the last two classes which are closer to the issues _reported_ to the user.

I'm not sure `DiagnosticUnderConstruction` is better than `BriefReport`, but `DetailedReport` can definitely be renamed to `FinalReport` for clarity.

> Assuming all of my guesses above are correct, I'm also not sure if there is a need for a `DetailedReport` class? Couldn't `ExtraInfo` just be added to the `BaseDiagnostic` or `GadgetDiagnostic` classes, which would eliminate the need of one class, and as a result make the diagnostic reporting engine a little bit less complex?

After thinking a bit more, I think the updated structure still makes sense, added a detailed explanation as a comment. Thank you for pointing this out!

https://github.com/llvm/llvm-project/pull/135662