Diff Detail
- Build Status
Buildable 9528 Build 9528: arc lint + arc unit
Event Timeline
Thank you for doing this. These were causing warnings with some compilers when built on PowerPC because the sources were just empty (macro-guarded). Not compiling them at all is a much cleaner solution.
LGTM but I am far from an authority on this part of the code so I'll let the approval come from one of the other reviewers.
Looks OK to me, but I haven't worked on this part of code either, so I think someone else should approve it.
Also, I think that if you're splitting them up, it'd also logical to move them into a subdirectory, x86-common maybe.
compiler-rt/lib/builtins/CMakeLists.txt | ||
---|---|---|
219 | This and the following files have only: #if !_ARCH_PPC so I suppose it's currently included on more targets than x86. If it's really not useful there, I think it'd be better to update the #ifs first (and preferably get the author to review). |
I've cleaned up this patch a bit. Now the only files that are in the x86_ARCH group are those that require 80 bits floats and cpu_model.c. Tests for all of these were already disabled on arm and powerpc (because neither has 80-bit floats), so we knew these library functions don't work on arm, but we compiled them anyway. It makes more sense to just not compile them at all if we know they require something the target architecture doesn't support.
I'm cc'ing some relevant people on this. The arm folks can comment on this patch (and I will add them to review) if there is a problem.
I'm reluctant to do this because that would require adding another layer to the file the filtering code around line 526, and that already has some special casing and is a bit hard to follow. Nevertheless, if you insist, I will.
compiler-rt/lib/builtins/CMakeLists.txt | ||
---|---|---|
219 | As discussed in D36764, these all assume 80-bit floats, and therefore are only useful on x86 (and ancient 68ks)--and, in fact, the tests for these functions are disabled for everything but x86. I have left these #ifs in for now, but will remove them after this patch gets submitted (and add comments about them needing 80-bit floats). Otherwise, powerpc builds will be broken. |
compiler-rt/lib/builtins/CMakeLists.txt | ||
---|---|---|
483 | why these files were not used before? should the change be in a separate patch? or update the patch title/summary as well? |
compiler-rt/lib/builtins/CMakeLists.txt | ||
---|---|---|
219 | Please do not remove the guards. We're using them in other arch-specific files as well. Instead, update them to state the correct targets. |
I'm not going to block this but I agree with others that the PPC changes look like they belong in a separate commit.
This and the following files have only:
so I suppose it's currently included on more targets than x86. If it's really not useful there, I think it'd be better to update the #ifs first (and preferably get the author to review).