[PATCH] D138019: [ARM][AArch64] De-template TargetParser types

[Tomas Matheson via Phabricator via llvm-commits]

tmatheson marked an inline comment as done.
tmatheson added inline comments.


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:25
 
 namespace AArch64 {
 
----------------
tschuett wrote:
> There is an AArch64 and an ARM namespace. 
Assuming this is in reply to @lenary, let me know if not.


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:86
 
-template <typename T> struct ArchNames {
+struct ArchNames {
   const char *NameCStr;
----------------
lenary wrote:
> is this in a different namespace to the arm version of this struct?
> 
> also, why does this class have a plural name? It doesn't represent a list, it represents an entry in a list? is this something we can fix now?
Yes it's llvm::AArch64::ArchNames.

I didn't name it so I don't know, it was copied verbatim from ARMTargetParser. Outside the scope of this patch...


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:135
 // When this becomes table-generated, we'd probably need two tables.
-template <typename T> struct CpuNames {
+struct CpuNames {
   const char *NameCStr;
----------------
lenary wrote:
> I think this also needs to be in a namespace so it doesn't collide with the ARM variant?
> 
> I have similar issues with the pluralness of this class name.
It's llvm::AArch64::CpuNames.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138019