[haiku-commits] Re: haiku: hrev46660 - build/jam src/system/libroot/posix/glibc/stdio-common
- From: Jonathan Schleifer <js-haiku-commits@xxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Sun, 12 Jan 2014 17:24:34 +0100
Am 12.01.2014 um 14:34 schrieb Ingo Weinhold <ingo_weinhold@xxxxxx>:
> I would have preferred immediate integration in configure, but anyway...
At that point, Haiku didn't even build completely with Clang, so adding that
configure option would most likely have caused bug reports about problems that
are already well known.
For now, it's just so that you have a way to at least build parts with Clang
and test them, without having to manually comment and uncomment stuff ;).
> CLANG is not a good variable name. As you may know, jam imports all
> environment variables as jam variables, so our build system generally uses
> certain name spaces for variables (HAIKU_*, HOST_*, TARGET_*) to avoid
> accidental clashes. In this case HAIKU_CLANG (or probably better
> HAIKU_CLANG_VERSION) should be used. BuildSetup would map it to a respective
> TARGET_* variable.
Noted. I'll change that.
> Naming aside, this needs to be an architecture specific variable. Otherwise
> both architectures of a hybrid build (e.g. gcc2+clang) would be affected. For
> a similar reason your solution for the glibc compiler flags doesn't generally
> work: CCFLAGS is not an architecture specific variable.
So how would it be correct?
> I'm not that fond of this solution. If clang produces more warnings, either
> the warnings are helpful and the code should be fixed or the warnings need to
> be disabled. We chose the current approach for enabling -Werror, so that we
> can iteratively do that for more and more components, cleaning up warnings
> while making sure later code changes won't introduce new ones.
Using -Werror with Clang would result in not being able to build a single file
of Haiku. Clang has *a lot* warnings more than GCC. Yes, they all are useful.
But there is no point in erroring out on them right now.
The iterative approach you want to do is wrong, too: You should not use -Werror
and then disable all other warnings. You should use -Werror=error-type so only
that type of warning triggers an error. Otherwise you're hiding warnings
because you only want some of them to generate errors!
Another way why it would not work with using -Wno-error-type and keeping
-Werror: I tried that at one point and stopped when that list spanned 5 lines.
So, -Werror is definitely wrong as long as not all warnings have been fixed.
Until then, -Werror=error-type should be used.
> I'd like such a fine grained approach for clang as well. What I have in mind
> is adding a third, optional, parameter to EnableWerror, which can be a build
> feature specification, so that one can write, e.g.
>
> EnableWerror src add-ons accelerants 3dfx : : !clang ;
>
> to enable -Werror, except for clang. Obviously "clang" and "gcc" would need
> to be declared as build features for that to work. I haven't checked whether
> at this point the build feature code has already been initialized, but it
> should definitely possible to set things up this way.
That's not the right approach. Just enable -Werror for those type of warnings
which already are fixed. Clang and GCC use the same names for error types, so
that works. There is nothing wrong with displaying warnings. They should always
all be displayed. But only certain types should cause an error.
> As mentioned above, this doesn't work generally due to CCFLAGS not being
> architecture specific. ATM we don't have any configuration variable that
> could be used for this purpose. We'd either have to add one (or consequently
> a whole set for C, C++, and linker flags) -- though for performance reasons
> I'm a bit reluctant to do that lightly -- or choose a more explicit approach,
> i.e. define an architecture specific variable (e.g.
> HAIKU_GLIBC_CCFLAGS_<arch>) with the flags in question and add them in each
> concerned Jamfile (CCFLAGS += HAIKU_GLIBC_CCFLAGS_$(TARGET_PLATFORM_ARCH)).
>
> This seems less elegant, but I prefer it since at least it is more obvious
> where those weird flags come from.
Hm, that isn't really better than the old patch before which added it to each
file. I wanted to get rid of the need to add it to every single Jamfile. Code
duplication always seems awfully wrong to me.
I think we really need to have C, C++ and linker flags for each target platform
arch. I'm sure this won't be the last time something like that is needed.
--
Jonathan
Other related posts:
