[cfe-commits] r151889 - in /cfe/trunk/lib: Basic/Targets.cpp Frontend/InitHeaderSearch.cpp

[Chandler Carruth]

On Fri, Mar 2, 2012 at 3:57 AM, David Chisnall <csdavec at swan.ac.uk> wrote:

> On 2 Mar 2012, at 11:25, Chandler Carruth wrote:
>
> > If you're going to be touching Solaris header search logic, please
> factor it into the driver like the Linux header search logic. That requires
> a separate Solaris toolchain (if there isn't one already) and overriding
> some methods, but it should be straight forward. Both win32 and Linux are
> already factored so you can look at those as an example.
>
> i asked you twice in IRC about this and didn't get any answer either
> time...
>

Sorry, I've been busy. =[ I saw your question, but did not have time to
respond at the moment you asked it.


> The gcc libraries are in the pattern:
>
>
> /usr/gcc/{gcc_major}.{gcc_minor}/lib/gcc/{triple}/{gcc_major}.{gcc_minor}.{gcc_subminor}/[amd64/]
>
> How do I use the GCC search logic to find things in this layout?  It
> seemed to be pretty Linux-specific, but I'd prefer to reuse it.
>

It would require extending it somewhat. I'm not immediately certain the
best way. Either generalizing it to handle this case, or allowing a
toolchain to hook directly into it.

Still, even if you are unable to use the GCC detection logic, you can still
setup the header search from the driver. There are plenty of non-GCC header
search options set up from the driver with explicit paths not fundamentally
different from how you're setting up the paths here. The difference is it
keeps the toolchain-specific logic.

>> Unconditionally define __C99FEATURES__ when using C++ on Solaris.  This
> is a
> >> (hopefully temporary) work around for libc++ exposing C99-but-not-C++98
> >> features in C++98 mode.
> >
> > Test case? Please don't commit without test cases.
>
> I prefer not to commit meaningless tests.  I could check that
> __C99FEATURES__ is defined on Solaris, but that's not the important thing.


That certainly is *an* important thing, if not the only important thing.
The test isn't meaningless, it confirms and enforces the intent of this
patch: that the macro you mention is unconditionally available on Solaris.


>  The important test is whether a file compiled with -stdlib=libc++ but not
> -std=c++0x can be compiled.  Depending on the contents of Solaris system
> headers and libc++ headers, this may or may not require __C99FEATURES__ to
> be defined.  It is not possible to test this on any system that does not
> have both the libc++ and Solaris headers installed.
>
> All that adding a test for the __C99FEATURES__ macro would do is slow down
> execution of the test suite, without checking for anything meaningful.


I disagree. Such a test is trivial to inspect and be completely certain it
is correct. However, the toolchain logic and which function is called when
and where is not trivial to inspect and verify correct. The test enforces
that the patch does what it says on the tin.

Such a test would also ensure that subsequent changes or refactorings don't
(perhaps completely by accident) break the invariant you're setting up
here, or that if they do you and the author will both have a clear sign in
a test change that something has gone awry. Otherwise, some action at a
distance might break this and none of us would be the wiser until someone
on Solaris complained.

You'll find that there are many tests already checked into Clang that do
exactly this -- they verify and codify the expected preprocessor state for
various flags, configurations, and platforms. I don't think Solaris is any
different. It would be good to add comprehensive testing of the
preprocessor environment necessitated on Solaris.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120302/5fe88eaa/attachment.html>