[PATCH] D86644: [llvm-mca] Initial implementation of output serialization using JSON

Thu Jan 21 04:33:36 PST 2021

andreadb added inline comments.

================
Comment at: llvm/tools/llvm-mca/PipelinePrinter.cpp:16
 #include "Views/View.h"
+#include "llvm/Support/JSON.h"

----------------
andreadb wrote:
> Is it necessary to include this file here?
> Also, I cannot see that file in the diff. Could you please upload a full diff (assuming that this is not a phab user error... :) ).
> 
> My point is that there is nothing specific about this file which is dependent on any definitions other than those from View.h.
> 
> Also, some views might end up implicitly including it anyway.
> I noticed that most views tend to include InstructionView.h (which unfortunately I am unable to see in this diff). Maybe you could move this include in there?
> 
Nevermind. Forget about my second sentence.
JSON.h already exists and obviously it is not modified by this patch.
Apologies for the confusion.

The question about whether it should be included here is still valid though.
That being said, it is not an issue, so it is up to you whether to move that include elsewhere or not.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86644/new/

https://reviews.llvm.org/D86644