[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

As discussed in `#hurd`, a few additional comments. The Hurd codepath should first check if the triple is eligible, ie. minimizing any cost to the driver invocation for most targets, which are not going to be Hurd. In fact I would wrap it in `LLVM_UNLIKELY` but that's just a suggestion. Once you confirmed that the triple in question is the one you're looking for (from the suffix), you can alter the existing triple. The `std::string` should be avoided here since even a zero length `SmallVector` will guarantee not allocating anything unless used. This may be worth factoring out into a separate function entirely, and another important point is avoiding any sort of unneeded overhead unless you've confirmed that it's the Hurd triple.

In general, I've looked over it again and I'd ask to get rid of switches that can be replaced with `if` statements. If there's a need later, you can add it later. For now, there's no need so I would avoid it. Switch is not generally an easy construct for humans to parse. Avoid arrays with 1 element until needed as well please. You can always introduce further changes as they're needed, however, for now, I'm strongly against adding "clutter" because it may be useful in the future.

I'll end this comment with this: In general when structuring your code, the performance penalty for other targets when the conditions that can be easily tested are not met should pretty much be close to nonexistent. I would suggest keeping that in mind before when submitting revisions.


Repository:
  rC Clang

https://reviews.llvm.org/D54379