[PATCH] D147485: [Object] Refactor build ID parsing into Object lib.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Looks good, with a couple of nits.



================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h:18-21
 #include "Markup.h"
 
 #include <map>
 
----------------
Whilst you're working on this file, please delete the blank lines between the include lists. These prevent clang-format from reordering them automatically to match the LLVM style guide. (Either in this patch, or in a separate NFC commit, no need for review).


================
Comment at: llvm/lib/Object/BuildID.cpp:21-22
 
-namespace llvm {
-namespace object {
+using namespace llvm;
+using namespace llvm::object;
 
----------------
mysterymath wrote:
> jhenderson wrote:
> > Why have you changed this?
> To make the new code comply with the style guide: https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
> Since this is a refactoring change, I altered the existing definitions so the file would be internally consistent.
Huh, I didn't know about that style guide entry. I have some issues with it, but that's a separate topic.


================
Comment at: llvm/test/DebugInfo/symbolize-filter-markup-parse-fields.test:7
+CHECK-NOT: error: expected address; found '00'
 CHECK: error: expected address; found ''
 CHECK: error: expected address; found '42'
----------------
As as suggestion, now that you've expanded the CHECK-NOT lines, it would be good to indent the CHECK lines so that the error messages line up with the CHECK-NOT line errors, as demonstrated by the inline edit (you'd apply it to each CHECK line). This way, it'll be easier to spot by eye anywhere which differs unexpectedly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147485