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