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 cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits