User Details
- User Since
- Mar 14 2013, 3:16 PM (523 w, 5 d)
Today
Thanks for working on this fix! A few things:
I don't think the _BitInt test case should block this patch if that turns out to cause problems.
LGTM, though the change should come with a release note. Suggestion you can take or leave as you see fit: should we turn the places where we removed the null pointer check into assertions that the pointer is nonnull? Or are we hoping the crash will be sufficient to tell us when we've missed a case?
This will also need someone from the LLVM side to look at the LLVM-specific changes. Most of my comments were focused on the behavior of test cases, but there may be more comments coming for the code changes once I've got a better handle on the test behavior.
Yesterday
This lets offloading languages such as OpenMP use the system string.h when compiling for the host and then the LLVM libc string.h when targeting the GPU.
LGTM! Precommit CI is still falling over because of the libcxx bot. That issue is being actively investigated, but we don't have an ETA on it being resolved. I think these changes are sufficiently safe to land and watch for post-commit failures.
I think this LGTM, but I'm holding off on signing off until @shafik is satisfied with the part he was asking about. You should add a release note for the fix, too.
I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.
The changes here LGTM, but I'd appreciate it if a second set of eyes double-checked whether I missed anything, given the complexity of the changes. (So wait a day or so before landing in case other reviewers want to chime in.)
LGTM, but you should fix up the commit message when you land since the patch has changed away from what's described in the summary.
(Btw, if you no longer plan to work on a patch, it's considered good form to "abandon" the patch in Phabricator. You can do this by clicking the Add Action drop-down menu and selecting Abandon Changes.)
LGTM! Adding the clang-vendors group for awareness since this technically could break some downstream (I don't expect it to given that this was deprecated, but you never know).
These changes need a release note. Given that this has been deprecated, is there documentation that should have been removed as well?
Sat, Mar 25
Fixing a typo pointed out off-list (missed an "as").
Ping
Fri, Mar 24
LGTM!
Want to add a release note for this as well?
LGTM with a suggestion.
The changes LGTM (though I had a small nit), but you should add a release note when landing.
LGTM! (The changes need a release note, though.) Do you need someone to commit this on your behalf? If so, please let me know what name and email address you would like used for patch attribution. I can add the release note when I land, or you can update the patch to add a release note, either is fine by me.
LGTM!
This should also have a release note, eventually.
Thu, Mar 23
LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?
LGTM with a bit more testing.
LGTM
LGTM aside from a tiny variable naming nit I missed before.
The precommit CI failures are unrelated to your changes (the Debian one is related to flang failures and the libcxx one seems to be related to infrastructure), so no need to worry about them.
Wed, Mar 22
LGTM
LGTM!
I can see some minor utility to this for some kinds of libraries perhaps, but I'm on the fence about adding the attribute. Is there a reason we need the user to write this attribute at all? e.g., could we be smarter about inline function definitions rather than making this the user's problem?
This appears to be the same efforts that have been going on in https://reviews.llvm.org/D146041. Can you coordinate with @AryanGodara so that there's only one patch for this?
Where do you suggest I add the documentation?
Please also add a release note and add documentation for the builtin.
Thanks for this!
Tue, Mar 21
(Note, precommit CI on Windows still shows a valid failure.)
You should also add a release note to clang/docs/ReleaseNotes.rst so users know about the new functionality.
LGTM
LGTM with a nit and a question, but can you add a release note when you land to let users know about the changes?