On 10/23/13 14:22, Joseph S. Myers wrote:
On Wed, 23 Oct 2013, Iyer, Balaji V wrote:
Hi Jeff et al.,
Here is a link to the updated patch
(https://docs.google.com/file/d/0BzEpbbnrYKsSbVY2X3ZLUnd4OXM/edit?usp=sharing).
We have fixed all the issues that Joseph pointed out in
http://gcc.gnu.org/ml/gcc/2013-10/msg00090.html. We have also added
symbol versioning and have double-checked (using nm) that all symbols
are hidden unless we have explicitly allowed them to be public.
As I said in my previous comments, please follow libatomic, libitm,
libsanitizer or libvtv in using a configure.tgt file in the library's
subdirectory to describe what systems are supported. This is especially
important now that all toplevel patches need applying to three rather than
two repositories (GCC SVN, src CVS, binutils-gdb git) - anything logically
specific to one of those three should go in an appropriate subdirectory
whenever possible, to reduce the number of changes needing syncing to
multiple places.
It also just makes sense from a modularity point of view. Whether or
not cilkrts is supported is a property of cilkrts and thus code to
detect that and "do the right thing" should live within the cilkrts
directory.
(Yes, there's lots of configuration specific to newlib/libgloss, binutils,
gdb or individual GCC libraries that's still hardcoded in the toplevel
configure.ac and should move to such configure.tgt or similar files in
subdirectories. Patches moving it to such files are certainly welcome.
But at least we shouldn't add new directories with details at toplevel of
what targets they support.)
Agreed. There's a lot of cruft up there that needs to move down into
the subdirectories. Unfortunately there's not many people actively
working to address these maintenance issues.
I didn't see anything else grossly wrong. I think once the
configure.tgt stuff is addressed, this patch is good to go. As
previously discussed, don't actually check it in until the final
approval is in place for the keywords.
jeff
