This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Move __CORRECT_ISO_CPP_WCHAR_H_PROTO closer to wchar.h include
Needs ReviewPublic

Authored by rupprecht on May 10 2023, 4:00 PM.

Details

Reviewers
iana
Group Reviewers
Restricted Project
Summary

This is only necessary for wchar.h, so only set it if we're including that.

Diff Detail

Event Timeline

rupprecht created this revision.May 10 2023, 4:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 4:00 PM
rupprecht requested review of this revision.May 10 2023, 4:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 4:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rupprecht updated this revision to Diff 521131.May 10 2023, 4:02 PM
  • Fix some indentation
iana added a comment.May 10 2023, 4:05 PM

Either way is fine with me. I didn't think about wchar.h, maybe better to leave that one the way it is to be on the safe side, and leave __mbstate_t.h the way you have it already for consistency.

iana accepted this revision.May 10 2023, 4:05 PM

I forgot about this patch but I won't be around to watch it go through buildbots etc., so I'll plan to land it as soon as I get back in a couple weeks.

Either way is fine with me. I didn't think about wchar.h, maybe better to leave that one the way it is to be on the safe side, and leave __mbstate_t.h the way you have it already for consistency.

I guess I also don't have a preference either way, but I think consistency is important -- trying to reason whether __CORRECT_ISO_CPP_WCHAR_H_PROTO affects one place but not the other is just real estate I don't want my brain to spend time on.

I wouldn't expect it to. At least in the sources I'm looking at, __CORRECT_ISO_CPP_WCHAR_H_PROTO should only affect the function signatures of wcschr, wcsrchr, wcspbrk, and wcsstr.

I forgot about this patch but I won't be around to watch it go through buildbots etc., so I'll plan to land it as soon as I get back in a couple weeks.

Does this main the breakage of D150015 will remain for a couple of weeks. If so I think we should revert D150015 and look at a good solution for you when you're back. WDYT?

I forgot about this patch but I won't be around to watch it go through buildbots etc., so I'll plan to land it as soon as I get back in a couple weeks.

Does this main the breakage of D150015 will remain for a couple of weeks. If so I think we should revert D150015 and look at a good solution for you when you're back. WDYT?

No, this change itself should be NFC. D150015 is a fix for D148542. So please don't revert D150015, unless it's part of also reverting D148542 if you choose to do that for https://github.com/llvm/llvm-project/issues/62523.

D148542 looks like a bug fix for something else, so I don't know if it's a good idea to revert. Check with @iana I guess?