[PATCH] D149941: [llvm][TargetParser] Create new Architecture base classes

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Stylistic comments, to help readers find the forest amid all the trees. :)
Probably should abbreviate `Architecture` as `Arch` everywhere, not just sometimes. It's a well-understood abbreviation.



================
Comment at: llvm/include/llvm/TargetParser/Architecture/ArchitectureHelpers.h:8
+//===----------------------------------------------------------------------===//
+#pragma once
+
----------------
LLVM style is to use [[ https://llvm.org/docs/CodingStandards.html#header-guard | header guards ]]
(comment applies to all headers)


================
Comment at: llvm/include/llvm/TargetParser/Architecture/ArchitectureHelpers.h:15
+public:
+  ArchitectureHelpers() = delete;
+
----------------
A `final` class with a deleted constructor and only static methods seems like a more complicated way to define a namespace; so please just use a namespace.


================
Comment at: llvm/lib/TargetParser/Architecture/CMakeLists.txt:1
+set(LLVM_REQUIRES_RTTI ON)
+add_llvm_component_library(LLVMArchitecture
----------------
The project prefers to have RTTI off. Where do you need this?


================
Comment at: llvm/unittests/TargetParser/Architecture/CMakeLists.txt:1
+set(LLVM_REQUIRES_RTTI ON)
+set(LLVM_LINK_COMPONENTS
----------------
The project prefers to have RTTI off. Where do you need this?


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

https://reviews.llvm.org/D149941