On 23 June 2015 at 15:50, Renato Golin <renato.go...@linaro.org> wrote: > > That's my point. If this patch makes sense on its own, we should mark them > all. I have no idea how to test it, though. >
I would probably have to create a new suite of unit tests, ensuring that for each ISO mode, only the correct set of library functions are marked as isLibFunction(). I don't see any machinery in Clang that can verify such things though, the builtins are lumped together. >> I tried to mention that with my link in Phabricator, but I realize I've >> failed to make it clear. There doesn't seem to be a good way of posting a >> logical patch set to Phab. > > > There isn't. That's why I do a (git diff -U999 master > patch) every time and > deal with patch-sets only locally. Gerrit seems to be a lot better on this, > but once you have submitted a patch set, squashing and rebasing gets *a lot* > worse, so I'm not fond of either way. Github is better. And you lose the contextual advantage distinct commits as well. I think I'd prefer to just be more explicit about relationships between my Phab reviews. In hindsight it was a mistake of mine not to market this patch as a distinct entity. Can we pretend that I did? :) >> Aside from http://reviews.llvm.org/D9912 relying on this change, I also >> think it's fine to go in on its own, since these builtins should be marked >> up as "F", given that they're listed in the C99 standard. Do you agree? > > AFAICS, they're not listed in C99, only their counterparts without the > __builtin_ are. IIUC, these builtins are the compiler trying to be smart > about implementation details, The point of the "F" flag is to mark exactly standard libm functions that have "__builtin_" prefixed to them. The details are in the comments in Builtins.def, particularly: // F -> this is a libc/libm function with a '__builtin_' prefix added. I still feel like this patch is just cleaning up some omissions in the C99 floating-point functions. > that's why Android shouldn't be fiddling with them in the first place. I agree that we're in __land, but GCC does happily accept this use-case, and I think there is a precedent for making compatibility changes like this. -- Charlie. _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
