[PATCH] D84519: [llvm-objdump][AMDGPU] Detect CPU string

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha added inline comments.


================
Comment at: llvm/include/llvm/Object/ObjectFile.h:330
   virtual SubtargetFeatures getFeatures() const = 0;
+  virtual StringRef getTargetCPUName() const { return StringRef(""); };
   virtual void setARMSubArch(Triple &TheTriple) const { }
----------------
scott.linder wrote:
> Can you document this, noting that it returns an empty `StringRef` when no CPU can be detected? I am also curious why you dropped the `Optional`, it seemed like a nicer interface that made the documentation of the "empty" case part of the code. I suppose in C++ there is no way to elide the extra storage for the `Optional`, but I would still think it is worth using.
> 
> I would also prefer to drop `Target` from the name, as `getArch`/`getFeatures` do not mention `{Sub}Target`.
Since `getArch` / `getFeatures` were not using `Optional`, I thought maybe the convention should be maintained. But I do agree that `Optional` is the more //self documenting// interface in a way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519