[PATCH] D153376: Introducing llvm-cm: A Cost Model Tool

[Dayann D'almeida via Phabricator via llvm-commits]

JestrTulip added inline comments.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:117
+    WithColor::warning(errs(), "llvm-cm")
+        << "Failed to get section name: " << toString(SecNameOrErr.takeError())
+        << "\n";
----------------
jhenderson wrote:
> https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
> 
> Also, test case?
Regarding the test cases for aspects such as Section Name, a good portion of the error handling for disassembly was modeled off the error handling in tools such as objdump and mca. For this specific case, and many of those that are unaddressed within the program, I find it hard to come up with a way to properly test these occurrences. 
For example, this error requires a Section to exist within the file, but its name to not be found. 
If I could receive any ideas on how to properly test situations like this, i'd be very grateful. 




================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:75
+class ExitIf {
+   public:
+    ExitIf(bool Cond) : Condition(Cond) {}
----------------
jhenderson wrote:
> jhenderson wrote:
> > Have you run clang-format on your new code? Because this doesn't look formatted according to the standard format.
> Ping - not addressed.
I've run git clang-format on the most recent patch.  Just to make sure I'm doing it correctly: 
I first git added all the modified files, 
I then ran git-clang format from the root of my repo 
I then received the message "clang-format did not modify any files" 
 


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:70
+// Class for error handling.  
+class ExitIf {
+   public:
----------------
jhenderson wrote:
> ondrasej wrote:
> > mtrofin wrote:
> > > nit: `class ExitIf final`
> > > 
> > > upon looking more closely at the usage pattern - @ondrasej , why raii and not just a function call? raii will exit at scope exit, which is undesirable (the goal is to exit right away). A function call "just works" because `exit(1)`, no?
> > As discussed offline yesterday: RAII is there to make the streaming operators after `ExitIf()` work.
> > 
> > With RAII, and with the intended use (`ExitIf(Cond) << "Some message";`), the following happens:
> > 1. a temporary `ExitIf` instance is created and takes note of `Cond`,
> > 2. all streaming operators are applied to the temporary (collecting the messages),
> > 3. it's a temporary value (there's no variable name), so its scope is limited to the statement where it appears.
> > 
> > Basically, it processes the chain of `<<`, and then immediately exits.
> Why do you need RAII for this? You could just do string concatenation to get your final message, and pass that into a function that prints the message and then calls `std::exit`.
The reason for not doing string concatenation to get the message and passing the result into a function is mainly due of the reduced overhead (specifically, the temporary string objects) the class option provides.  With string concatenation, in all cases, we must perform extra object allocations to get the final message. 
However, with this implementation, we only assemble the message into the stringstream when there's an error. 
I realize this was not exactly what original implementation did,  but I've updated it alongside my most recent update. If this implementation is not preferred, I am perfectly able to accommodate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376