[llvm-commits] [llvm] r123990 - in /llvm/trunk: include/llvm/ADT/Triple.h lib/Support/Triple.cpp unittests/ADT/TripleTest.cpp

[Duncan Sands]

Hi Renato,

> @@ -84,6 +84,7 @@
>
>     case Apple: return "apple";
>     case PC: return "pc";
> +  case NoVendor: return "none";

if there is no vendor then you could just output nothing, resulting
in triples like eg x86_64--linux-gnu
However if triples with "none" are floating around in the real world then
this is OK.

>     }
>
>     return "<invalid>";
> @@ -109,6 +110,7 @@
>     case Win32: return "win32";
>     case Haiku: return "haiku";
>     case Minix: return "minix";
> +  case NoOS: return "none";

Likewise.

>   void Triple::Parse() const {
> @@ -344,7 +360,12 @@
>     Arch = ParseArch(getArchName());
>     Vendor = ParseVendor(getVendorName());
>     OS = ParseOS(getOSName());
> -  Environment = ParseEnvironment(getEnvironmentName());
> +  if (OS == NoOS) {
> +    // Some targets don't have an OS (embedded systems)
> +    Environment = ParseEnvironment(getOSName());
> +  } else {
> +    Environment = ParseEnvironment(getEnvironmentName());
> +  }

I don't think you should be doing this at all.  Parse should just parse
things and not do any reasoning.  It's up to Normalize to sort things
out.

>
>     assert(isInitialized()&&  "Failed to initialize!");
>   }
> @@ -411,7 +432,13 @@
>           break;
>         case 2:
>           OS = ParseOS(Comp);
> -        Valid = OS != UnknownOS;
> +        // Some targets don't have an OS (embedded systems)
> +        if (OS == NoOS) {
> +          Environment = ParseEnvironment(Comp);
> +          Valid = Environment != UnknownEnvironment;
> +        } else {
> +          Valid = OS != UnknownOS;
> +        }

I don't think you should be doing this either.  Why do you touch this?  It seems
quite wrong to me.  If you want to do special case logic don't do it here, do it
below at the point that says:

   // Special case logic goes here.  At this point Arch, Vendor and OS have the
   // correct values for the computed components.


>           break;
>         case 3:
>           Environment = ParseEnvironment(Comp);
> @@ -450,6 +477,9 @@
>             for (unsigned i = Idx; i<  Components.size(); ++i) {
>               // Skip over any fixed components.
>               while (i<  array_lengthof(Found)&&  Found[i]) ++i;
> +            // Fix problem when Components vector is not big enough
> +            if (i>= Components.size())
> +              Components.push_back(StringRef(""));

This is a sign that something is wrong - I suspect this fix is a bandaid
and the problem would go away if you removed the logic trying to relate OS
and environment earlier in the normalization logic.

In short, I think you should at most just add NoOs and NoVendor in a trivial
way, without poking at Parse or Normalize at all.  At worst any special case
logic (and why do you need any?) should go at the point I mentioned above.

Ciao, Duncan.