This is an archive of the discontinued LLVM Phabricator instance.

[libc++] "Merged wording" for D98077 and D96986
ClosedPublic

Authored by Quuxplusone on Mar 5 2021, 6:45 PM.

Details

Summary

This is my attempt to merge D98077 (bugfix the format strings for Windows paths, which use wchar_t not char) and D96986 (replace C++ variadic templates with C-style varargs so that __attribute__((format(printf))) can be applied, for better safety). Those two patches merge-conflict with each other, and also I had some pretty heavy "requested changes" on D96986, so I decided to just show what I mean directly.

The one intentional functional change here is in __create_what. It now prints path1 and path2 in square-brackets and double-quotes, rather than just square-brackets. Prior to this patch, it would print either path double-quoted if-and-only-if it was the empty string. Now the double-quotes are always present. I doubt anybody's code is relying on the current format, right?

(Added @EricWF as the original author of this code, in 9158bfd32ebd4)

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Mar 5 2021, 6:45 PM
Quuxplusone created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 5 2021, 6:45 PM

And of course as soon as I look at the diff I spot a missing va_end. :) Hopefully fixed now.

Quuxplusone edited the summary of this revision. (Show Details)Mar 5 2021, 6:58 PM
Quuxplusone added a reviewer: EricWF.
Quuxplusone added a subscriber: EricWF.
joerg added a comment.Mar 5 2021, 7:00 PM

That covers my case. I'm not completely happy with the need to rethrow, but the alternatives are much worse. It's a bit sad that we can't optimize it away, too.

Adjust expected output in tests.
Guard _LIBCPP_FORMAT_PRINTF with defined(__GNUC__) || defined(__clang__) so as to pick up clang-cl support.

curdeius requested changes to this revision.Mar 8 2021, 8:03 AM

Apart from the fact that it fails with exceptions disabled, I have no other remarks. Generally it's ok.
I'll have another look at it when it gets green.

This revision now requires changes to proceed.Mar 8 2021, 8:03 AM

Guard try/catch for the no-exceptions build.
Fix a few things clang-format was complaining about. But I'm not making clang-format totally happy, and I think that's okay because some of what it's asking for is dumb.

curdeius accepted this revision as: curdeius.Mar 8 2021, 10:01 AM

LGTM pending CI.

mstorsjo accepted this revision.Mar 10 2021, 12:36 AM
mstorsjo added subscribers: Mordante, zoecarver.

LGTM from my point of view, but this still needs another approver (or can @Quuxplusone self-approve for the second approval)? Or can @Mordante or @zoecarver have a look?

libcxx/include/__config
1462

Wouldn't it be better use 2 named arguments instead of .... The latter isn't officially part of C++98 and the attribute requires 3 arguments.

libcxx/src/filesystem/filesystem_common.h
81

Not too fond of the number of static_casts and the "TODO", but it seems they were already there.

88

For consistency can you remove std::?

libcxx/include/__config
1462

I had thought I was copying style from elsewhere in this file, but I guess only _LIBCPP_DIAGNOSE_WARNING/_LIBCPP_DIAGNOSE_ERROR actually use ... for this purpose. So the only benefit I'm getting from ... is that I don't have to worry about naming or whitespace conventions.
How do we feel about

#define _LIBCPP_FORMAT_PRINTF(a, b)   \
  __attribute__((__format__(__printf__, a, b)))

We already use a, b as macro parameter names on line 227. If this naming/whitespace looks acceptable, I don't mind making this change.

FWIW, I claim C++98-portability is not a concern, given that we already use ... specifically in C++98 mode, e.g. in #define static_assert(...) on line 898.

joerg added inline comments.Mar 10 2021, 1:46 PM
libcxx/include/__config
1462

Macro arguments are limited to the expansion as scope and disappear afterwards. They don't interact with tokens outside the macro expansion. I don't think compatibility with C++98 is a concern here, but there is no reason to give up the checking on the number of arguments here. E.g. in NetBSD they are called fmtarg and firstvararg to make it self-documenting.

Mordante added inline comments.Mar 11 2021, 9:56 AM
libcxx/include/__config
1462

You're right we already use ... in C++98 mode. I prefer named arguments if the number required is fixed. I like the ones @joerg proposes. I had to read the manual to see what the syntax of the attribute was.

libcxx/include/__config
1462

I strongly prefer not fmtarg, firstvararg, but I'll do a, b.
(Note that as with any attribute that names parameters by index, there are issues such as whether to count this and whether to index starting at 0 or 1. I also would have to look up the exact semantics, or better, cut-and-paste from somewhere that already got it right.)

Addressed @Mordante's comments. Ping for approval!

Quuxplusone marked an inline comment as done.Mar 12 2021, 6:32 AM
Quuxplusone added inline comments.
libcxx/src/filesystem/filesystem_common.h
88

Done (and also on line 71). This file does contain some std::s, on declval and array, but not on string. (Personally I wish it would put std:: on everything, just like user code should. We're not doing anything "special" in this file that requires us to deviate from the usual good practice; the current use of unqualified string is just inessential mental noise IMO.)

Also regarding this one, before landing it, it'd be good with a more descriptive subject - this one makes sense during review, but makes less sense when looking back at the lone commit afterwards.

Quuxplusone marked an inline comment as done.Mar 13 2021, 6:22 AM

@mstorsjo: I'm planning to include the revision summary ("This is my attempt to merge ... format, right?") in the commit message. I'm failing to come up with a good title line. Maybe [libc++] Apply attribute(format(printf)) and Win32 fixes to filesystem helpers?

@mstorsjo: I'm planning to include the revision summary ("This is my attempt to merge ... format, right?") in the commit message. I'm failing to come up with a good title line. Maybe [libc++] Apply attribute(format(printf)) and Win32 fixes to filesystem helpers?

I guess that's as concise as it gets, yeah - that sounds good to me.

Mordante accepted this revision.Mar 16 2021, 11:36 AM

LGTM!

libcxx/src/filesystem/filesystem_common.h
88

I don't mind adding std::. Just as long as the code is consistent.

This revision is now accepted and ready to land.Mar 16 2021, 11:36 AM