[PATCH] D10839: code refactoring on ARMTargetInfo class

Alexandros Lamprineas alexandros.lamprineas at arm.com
Mon Jul 13 03:40:47 PDT 2015


labrinea added inline comments.

================
Comment at: lib/Basic/Targets.cpp:33
@@ -32,2 +32,3 @@
 #include <memory>
+#include <locale>
 using namespace clang;
----------------
rengolin wrote:
> Why do you need to include <locale>?
I thought isalnum needed that.

================
Comment at: lib/Basic/Targets.cpp:4198
@@ -4199,3 +4197,3 @@
   ARMTargetInfo(const llvm::Triple &Triple, bool IsBigEndian)
       : TargetInfo(Triple), CPU("arm1136j-s"), FPMath(FP_Default),
         IsAAPCS(true), HW_FP(0) {
----------------
rengolin wrote:
> Since we're setting up the default CPU on the Arch, we don't need it to be initialised here.
> 
> Also, "arm1136" looks like the wrong value here, since the default CPU for ARM when nothing is known is "arm7tdmi".
> 
> I risk to say that this value was never taken into consideration because the architecture was never empty (errors dealt with before), so I'd leave CPU uninitialized here, and assert down there if CPU == nullptr.
The value "arm1136j-s" was taken into consideration. A regression will appear if we don't set it, since ARM_ARCH_6J won't be defined as expected (tools/clang/test/Preprocessor/init.c). I don't like this hard-coded string either but it was there before my patch. Do you think we should change the test?

================
Comment at: lib/Basic/Targets.cpp:4236
@@ -4216,4 +4235,3 @@
 
-    // FIXME: Should we just treat this as a feature?
-    IsThumb = getTriple().getArchName().startswith("thumb");
+    IsThumb = (ArchISA == llvm::ARM::IK_THUMB);
 
----------------
rengolin wrote:
> This should also go inside setArchInfo()
IsThumb depends on ArchISA. ArchISA should be initialized based on triple and never change. Updating IsThumb in setArchInfo() will not change its value.

================
Comment at: lib/Basic/Targets.cpp:4430
@@ +4429,3 @@
+        return "";
+      for(char c : std::string(CPUAttr))
+        assert( (std::isalnum(c) || c == '_') &&
----------------
rengolin wrote:
> This validation doesn't belong here. If ARMTargetParser returned a valid CPU name, the name is valid.
> 
> If the name is wrong, ARMTargetParser is wrong and should be fixed instead.
That's the assertion I introduced to catch the buildbot error (whitespace expected after macro definition). I am still surprised that it passed the test this time since the only thing I added is three cases to the switch clause (ARMV6HL, AK_ARMV7L, AK_ARMV7HL). Those were the only cases of illegal characters ( '-' ) in CPUAttr not handled.


http://reviews.llvm.org/D10839







More information about the cfe-commits mailing list