[PATCH] D10839: code refactoring on ARMTargetInfo class

[Renato Golin]

rengolin added inline comments.

================
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) {
----------------
labrinea wrote:
> 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?
No. Add another FIXME outlining the problem, hinting where to fix it properly. This patch is already too big and long lasting for its own good. :)

================
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);
 
----------------
labrinea wrote:
> 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.
Good point.

================
Comment at: lib/Basic/Targets.cpp:4430
@@ +4429,3 @@
+        return "";
+      for(char c : std::string(CPUAttr))
+        assert( (std::isalnum(c) || c == '_') &&
----------------
labrinea wrote:
> 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.
Regardless, this is not the place to check for this kind of problem.

Maybe, on a separate patch, we can add that check to ARMTargetParser, which is the right place.

Also, using std::string is a bit heavy handed for this task.


http://reviews.llvm.org/D10839