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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
