Address comments

I don't plan to work on this any time soon, so I'll abandon it to clean up the review queue.

Try to fix CTry to fix CII

Try to fix CI

Try to fix CI

Try to fix CI

Try to fix CI

Try to fix CI

Try to fix CI

Try to fix CI

Try to fix CI

Abandoning, since it seems like we want to extend diagnose_if instead.

Rebased

Address comments

Try to fix CI

Try to fix CI

Try to fix CI

Let's land the current stack and move this PR to GitHub.

We're transitioning to GitHub and I think we should have enough time to git this at least as far as it has been, so let's land this and continue reviews on GitHub.

Address comments

I just tried it on my local system and the test passes with both clang and GCC. This strongly indicates that everything works except testing in the docker image. Given that this is purely a CI pro I'm strongly against removing anything here.

In D144364#4636494, @Joy12138 wrote:

It seems that the long double is a type with a size of 12 bytes on MinGW (DLL, i686). This conflicts with the current implementation. Vector extension requires the input parameter of vector size to be a power of 2 and a multiple of the size of the element type. 12 bytes cannot meet both requirements simultaneously. Should we temporarily exclude long double on MinGW (DLL, i686)? I think it would be more appropriate to add support for it when implement platform specialization optimization. @philnik

Address comments

Try to fix CI

Rebased

In D158922#4635043, @zetafunction wrote:

In D158922#4635027, @philnik wrote:

In D158922#4634975, @zetafunction wrote:

Is there updated documentation for best practices for doing that? I was following https://libcxx.llvm.org/Contributing.html.

We have just started the move, so I don't think there is any documentation yet. You should be able to just fork the llvm-project and then open a PR against the main repo.

Thanks, I've gone ahead and opened the PR at https://github.com/llvm/llvm-project/pull/65177? Since you commented, would you be willing to take a look at it? It hasn't been quite a week yet (per https://llvm.org/docs/Contributing.html#how-to-submit-a-patch) but I'm not sure what the best way to proceed here is either, since this is my first libc++ patch.

In D158922#4634975, @zetafunction wrote:

Is there updated documentation for best practices for doing that? I was following https://libcxx.llvm.org/Contributing.html.

Try to fix CI

Address comments

Is there still interest in pursuing this? If not, could you abandon the patch?

[Github PR transition cleanup]

Is there still interest in pursuing this? If not, could you abandon this?

Since we are moving to GitHub PRs and there hasn't been any activity here yet, could you move this patch over?

[Github PR transition cleanup]

Abandoning for now, since this doesn't seem to be used much in tooling.

Abandoning for now, since this requires some sort of compiler fix to work properly and this won't happen within the next month or so.

Since we are moving to GitHub PRs and there hasn't been any activity here yet, could you move this patch over?

Since we are moving to GitHub PRs and there hasn't been any activity here yet, could you move this patch over?

Could you abandon this, since there is a PR open on GitHub?

Update ignore_format.txt

I think we should try to rebase, as @Mordante suggested. If that doesn't help I'd just go for // XFAIL: gcc-13ing the tests for now. If the bug still exists in GCC 13 it would be great if you could try to get a small reproducer and file a bug against GCC.

In D157776#4622524, @AMP999 wrote:

@philnik gentle ping!

Re "there is no requirement in the standard," I think the use of the term "move constructed" in https://eel.is/c++draft/stringbuf#members-10.sentence-1 is that requirement. The original paper P0408 also contains this test case https://github.com/PeterSommerlad/SC22WG21_Papers/blob/master/workspace/Test_basic_stringbuf_efficient/src/Testp0407-p0408-efficientStringstreams.cpp#L105-L112 which won't succeed unless we transfer ownership here. Transfer-of-ownership is clearly P0408's intent.

A moved-from string has an unspecified state, so there is no way to check what actually happened.

In D158749#4621858, @Mordante wrote:

In D158749#4619918, @philnik wrote:

Shouldn't this be possible to actually test? If not, please add a comment why this change was done.

Do you have a suggestion how to test this. It depends on overflowing streamsize. This is an alias of ptrdiff_t.

Ah sorry, I missed that. I though it depends the char traits. In that case I'm happy with a short explanation in the commit message. Sorry for the churn.

Shouldn't this be possible to actually test? If not, please add a comment why this change was done.

In D157572#4617504, @aaron.ballman wrote:

In D157572#4614561, @cjdb wrote:

In D157572#4613622, @aaron.ballman wrote:

In D157572#4612141, @cjdb wrote:

I don't dislike it, but I am a bit concerned about misuse being noisy.

So you're concerned that a library author uses diagnose_if to add a diagnostic to a warning group that makes the diagnostic seem too chatty, so the user disables the group and loses the compiler's diagnostics? Or are there other kinds of misuse you're worried about?

More or less the former. I don't know if it'll actually manifest in practice though, I don't see many diagnose_if diagnostics to begin with.

I think the former is sort of a "doctor, doctor, it hurts" situation; the issue isn't really with diagnose_if, it's with the library author's use of it. But at the same time, I can see why it would be beneficial to be able to work around it if needed.

As much as I hate suppressing diagnostics, I think there needs to be a way to suppress the diagnose_if forms of warning without suppressing something that the compiler would otherwise generate. Something like:

• -Wno-deprecated: suppresses anything that -Wdeprecated would turn on.
• -Wno-deprecated=diagnose_if: just the ones flagged by diagnose_if.
• -Wno-deprecated=non-diagnose_if: complement to #2.

(and similarly for -Wno-error=.)

I'm not sure about the system header knob though: [[deprecated]] and [[nodiscard]] still show up even when the declaration is in a system header?

Correct, those will still show up when the declaration is in a system header but not when the use is in a system header: https://godbolt.org/z/PjqKbGsrr

Right, my question was more "what is this knob doing?"

Ah! We have various knobs on our diagnostics, like:
ShowInSystemHeader -- https://github.com/llvm/llvm-project/blob/2dc6281b98d07f43a64d0ef34405d9a12d59e8b6/clang/include/clang/Basic/DiagnosticSemaKinds.td#L7818 (if used, the diagnostic will be shown in a system header)
SFINAEFailure -- https://github.com/llvm/llvm-project/blob/2dc6281b98d07f43a64d0ef34405d9a12d59e8b6/clang/include/clang/Basic/DiagnosticSemaKinds.td#L93 (if used, the diagnostic causes SFINAE to fail in a SFINAE context)
DefaultIgnore -- https://github.com/llvm/llvm-project/blob/2dc6281b98d07f43a64d0ef34405d9a12d59e8b6/clang/include/clang/Basic/DiagnosticSemaKinds.td#L141 (if used, the warning is off by default and must be explicitly enabled via its group)
DefaultError -- https://github.com/llvm/llvm-project/blob/2dc6281b98d07f43a64d0ef34405d9a12d59e8b6/clang/include/clang/Basic/DiagnosticSemaKinds.td#L262 (if used, the warning defaults to being an error but users can disable the error with -Wno)
(etc)

All of these have been of use to us as implementers, so it seems likely that these same knobs would be of use to library authors adding their own compiler diagnostics, so perhaps we should consider that as part of the design?

We currently have -Wuser-defined-warnings as the warning group for diagnose_if warning diagnostics, so I wonder if it would make sense to allow -Wno-deprecated suppresses anything that -Wdeprecated would turn on, while -Wdeprecated -Wno-user-defined-warnings would turn on only the compiler-generated deprecation warnings and not the diagnose_if-generated ones?

Oh neat, this simplifies things a lot!

I think it could make for a reasonable user experience.

I'm curious what @erichkeane thinks as attributes code owner, but personally, I like the idea of extending diagnose_if over the idea of adding clang::library_extension because it's a more general solution that I think will give more utility to our users. However, I also want to know if @philnik @Mordante @ldionne (and others) share that preference because I think they're going to be the guinea pigs^W^Wearly adopters of this functionality.

In D157572#4606513, @aaron.ballman wrote:

In D157572#4604595, @philnik wrote:

In D157572#4604482, @aaron.ballman wrote:

This allows standard libraries to mark symbols as extensions, so the compiler can generate extension warnings when they are used.

Huh, so this is basically the opposite of the __extension__ macro (which is used to silence extension warnings)?

I guess, kind-of. I never really understood the semantics of __extension__ though, so I'm not 100% certain.

It's used in system headers to say "I'm using an extension here, don't warn about it in -pedantic mode".

Hm, OK. I thought I tried to use it that way at some point and it didn't work in the way I expected.

I don't think we need to introduce a new attribute to do this, we already have diagnose_if. e.g., https://godbolt.org/z/a5ae4T56o would that suffice?

Part of the idea here is that the diagnostics should be part of -Wc++ab-extension.

Hmmm, okay. And I'm assuming -Wsystem-headers -pedantic is too chatty because it's telling the user about all use of extensions, not extensions being introduced by the library itself? (e.g., https://godbolt.org/z/Gs3YGheMM is also not what you're after)

Yeah, for libc++ we don't support -Wsystem-headers in any meaningful way and this doesn't achieve the effect I want.

I guess we could allow warning flags instead of just "warning" and "error" in diagnose_if that specifies which warning group the diagnostic should be part of. Something like __attribute__((__diagnose_if__(__cplusplus >= 201703L, "basic_string_view is a C++17 extension", "-Wc++17-extensions"))). I'm not sure how one could implement that, but I guess there is some mechanism to translate "-Wwhatever" to a warning group, since you can push and pop warnings. That would open people up to add a diagnostic to pretty much any warning group. I don't know if that's a good idea. I don't really see a problem with that other than people writing weird code, but people do that all the time anyways. Maybe I'm missing something really problematic though.

That's actually a pretty interesting idea; diagnose_if could be given another parameter to specify a diagnostic group to associate the diagnostic with. This would let you do some really handy things like:

void func(int i) __attribute__((diagnose_if(i < 0, "passing a negative value to 'func' is deprecated", "warning", "-Wdeprecated")));

But if we went this route, would we want to expose other diagnostic-related knobs like "show in system header" and "default to an error"? Also, the attribute currently can only be associated with a function; we can use this for classes by sticking it on a constructor but there's not much help for putting it on say a namespace or an enumeration. So we may need to extend the attribute in other ways. CC @cjdb as this seems of interest to you as well.

In D157572#4604482, @aaron.ballman wrote:

This allows standard libraries to mark symbols as extensions, so the compiler can generate extension warnings when they are used.

Huh, so this is basically the opposite of the __extension__ macro (which is used to silence extension warnings)?

I guess, kind-of. I never really understood the semantics of __extension__ though, so I'm not 100% certain.

Looks pretty good, but I'd like to get the ranges::equal optimization in again, since it's probably quite significant.

Address comments

Address comments

• Rebase
• Only add type_visibility("default") to the namespace

Fix formatting

Maybe I'm missing something, but it looks to me like you are disabling too many symbols here.

In D157058#4593597, @glandium wrote:

I'll be honest, this change is kind of annoying. The deprecation itself was added in clang 17, which is yet to be released. IOW, most code bases that are using the deprecated char_traits haven't even had the chance to see those deprecation warnings. And now they hard-fail to build with clang trunk...