In D147288#4237413, @royjacobson wrote:

There was some discussion of this last year in this review: https://reviews.llvm.org/D128750

It's such an edge case that I don't think we should lose sleep about it until/unless the committee finds time to clarify the issue.

Thanks so much for seeing this through; I'm unusually looking forward to rebuilding LLVM this weekend!

I'm going to keep this CL open till someone comments, but after reading the relevant GCC bug, I'm not sure it's worth committing anymore.

In D146358#4229120, @hazohelet wrote:

In D146358#4227938, @cjdb wrote:

In D146358#4204412, @tbaeder wrote:

"subobject named 'foo'" sounds a bit weird to me, I'd expect just "subobject 'foo'", but that's just a suggestion and I'll wait for a native spearker to chime in on this.

My expert brain likes subobject of type 'T', but it's very wordy, and potentially not useful additional info. I'm partial to subobject 'T' due to it being the thing we're more likely to say in conversation (e.g. "that int isn't initialised"). I've also put out a mini-survey on the #include <C++> Discord server, and will update in a few hours.

Hi, @cjdb thanks for your feedback and the mini-survey.
The objective of this patch is to print the subobject's name instead of its type when it is not initialized in constexpr variable initializations. Thus, I think it would be appropriate to add some more options to the survey like "subobject named 'foo' is not initialized" and "subobject 'foo' is not initialized" when the code looks like the following:

template <typename T>
struct F {
	T foo;
	constexpr F(){}
};
constexpr F <int>f;

In D146358#4204412, @tbaeder wrote:

"subobject named 'foo'" sounds a bit weird to me, I'd expect just "subobject 'foo'", but that's just a suggestion and I'll wait for a native spearker to chime in on this.

In D145201#4216567, @aaron.ballman wrote:

In D145201#4214395, @cjdb wrote:

swaps commits to see if that fixes CI (part 1)

Oh, if this is just stacked patches confusing our precommit CI, then it's okay. I hadn't realized this was a stacked patch.

LGTM from a diagnostic perspective!

And on this CL's side too

I think this corrects everything.

rebasing continues

swaps commits to see if that fixes CI (part 1)

swaps commits to see if that fixes CI

fixes breakage and adds FIXME

Thanks for working on this, it's much appreciated!

Thanks for working on this. I haven't contributed to libc++ in a very long time, so the project's policies may be different to what I'm asking for. Having said that, the following things would be good to do:

Would you be able to update the commit message to include a description that explains why this commit is necessary please?

LGTM pending other reviewers' commentary. Thanks for working on this!

D145438 was merged into D145284.

Merged into D145284.

merges in D145438. This patch is ready for review now.

In D129951#4179481, @ldionne wrote:

In D129951#4179467, @cjdb wrote:

I'm deeply disappointed that libc++ moved away from using __function_like: that was an important part of preventing niebloid misuse. It isn't conforming to treat niebloids as function objects, which is what __function_like prevented (I just checked std::ranges::next and it seems that __function_like was completely removed).

Oops, it seems like my reply got into a race condition with yours. I am curious though, why is it non-conforming to treat niebloids as function objects? It is certainly more lax than required by the Standard, however we are allowed to make those copyable, aren't we? Does the Standard say that we *have* to make them non-usable as objects? That would change things for sure (not with this patch, but we'd probably want to try re-introducing __function_like, and we'd file a bug report against libstdc++ to do the same).

In D129951#4178949, @philnik wrote:

In D129951#4178844, @cjdb wrote:

In D129951#4178154, @philnik wrote:

I don't think libc++ can adopt this without having to essentially duplicate our code, since GCC doesn't support __disable_adl (and AFAICT there is no coordination between GCC and Clang to add it to both).

I haven't had a lot of time to drive this in Clang, let alone GCC. Even if libc++ can't ultimately use it (which would be sad), there are other libraries that can. For example, Abseil has a similar attitude towards functions as Niebloids, and could wrap it behind a macro.

Abseil has the same support problem though AFAICT. In fact, most open source libraries don't just support clang.

In D129951#4178154, @philnik wrote:

I don't think libc++ can adopt this without having to essentially duplicate our code, since GCC doesn't support __disable_adl (and AFAICT there is no coordination between GCC and Clang to add it to both).

tidies up some stuff that I overlooked

adds dependency

fixes string goof

sorts artifacts so that they're output in index order

In D145178#4167103, @aaron.ballman wrote:

LGTM, this is incremental progress. Hopefully we won't be adding too many more RUN lines to this file though (sticking too many tests into one file is also tech debt).

fixes something that wasn't supposed to be changed in the rebase

actually commits the files

removes something from a future commit

removes redundant line

renames check identifiers

In D137458#3909343, @troyj wrote:

Also, it would be nice to have some numbers for the 'measurably faster' claim :)

Sure. Here's an example of a library change that started using builtins for __make_integer_seq and __type_pack_element https://github.com/facebook/fatal/commit/58102a3f7e66ad122d7d3335c446399b09d5085e where there was a 1.8% speedup for a file that spent 70 seconds in the front end. It also reduced the peak memory usage of the front end by 0.5%. We have reason to believe that a __decay builtin would produce similar benefit. Unfortunately, all of the intrinsics in https://reviews.llvm.org/D116203 were implemented with parentheses syntax instead of as builtin templates, which makes it more difficult. Ideally, we'd like to see all of these implemented as builtin templates, but __decay is the first one that we're proposing.

I'm not familiar with the mangling issue that you mentioned. I'll look into it more.

In D135341#4111673, @ldionne wrote:

I've been looking at implementing reference_constructs_from_temporary & friends and this would be sweet to have. Is this blocked on something specific?

Ping @aaron.ballman

Ping @aaron.ballman @rsmith

responds to feedback

• updates patch so that it meets what Aaron and I want, also meets most of Richard's feedback
• updates commit message

In D141836#4080778, @echristo wrote:

I would hypothesize that the number of platforms where compiler-rt is used to link aarch64 code is greater than the number that don't and there's work to make linking with compiler-rt (https://github.com/llvm/llvm-project/tree/main/llvm-libgcc) in order to replace uses on other platforms. In addition, while my employment meant I couldn't add this to libgcc in the past there's no reason why it couldn't be added as well. Has anyone tried?

Thanks for working on this! Needs a bit of work, but we should be able to get this in very soon methinks.

Excellent, let's wait for @denik's feedback before merging, but this LGTM. Thank you for the patch!

LGTM, thanks for fixing!

In D128372#3991516, @MyDeveloperDay wrote:

Ironically as an aside... looking back at the review because of the discource article.

The whole reason why I added the [[nodiscard]] checker to clang-tidy 4 years ago was to cover exactly this use case...and many others like it where users have a function which returns a boolean and takes no arguments are pointer or reference (empty() included)

https://reviews.llvm.org/D55433

Shouldn't people be using this checker to add nodiscard and let the compiler to the heavy lifting. Was there a use case as to why that solution doesn't solve this?

LGTM

In D138939#3967397, @tschuett wrote:

I do not ask you to do anything! I just noticed that you add a lot of FormatXXXDiagnostic functions. An alternativ design is to have one FormatDiagnostic function with a mode parameter. Then you can decide whether to print legacy or user-oriented reasons.

If next year you invent another diagnostic, you can extend the enum and the FormatDiagnostic function.

In D138939#3966938, @tschuett wrote:

Maybe the kind/amount of information printed ( DiagnosticMode ) and the output device (console/sarif) are orthogonal issues.

Still it would nice to be able to toggle the diagnostic mode for users/testing / A/B.

In D138939#3965985, @tschuett wrote:

Then Sarif was a distraction. Still to reduce boilerplate and for A/B testing:

enum class DiagnosticMode {
  Legacy,
  UserOriented,
  Default = Legacy
}

In D138939#3964677, @erichkeane wrote:

In D138939#3964439, @erichkeane wrote:

In D138939#3964404, @cjdb wrote:

In D138939#3963496, @erichkeane wrote:

In D138939#3963473, @tschuett wrote:

Maybe void FormatDiagnostic(SmallVectorImpl<char> &OutStr, DiagnosticMode mode)instead of void FormatDiagnostic(SmallVectorImpl<char> &OutStr)?
To make the transition easer and future proof.

I like this idea. Perhaps with DiagnosticMode being a 3-way enum:

enum struct DiagnosticMode {
  Legacy,
  Sarif,  
  Default = Legacy
}

I like the idea in particular, since it makes a command line flag to modify "Default" to be whichever the user prefers pretty trivial.

There's already a flag for this: -fdiagnostics-format=sarif. Why do we need a second diagnostic mode flag?

Ah, oh... is the Sarif formatting being done with a new formatter? That seems unfortunate, since folks using the other formatters won't be able to use the user friendly formats.

I've been alerted offline that I am misunderstanding the Sarif proposal, and where this is going. I'll note that I wasn't present/invited at the calls where all of this was discussed, so I am admittedly not completely up to date. The above concern shouldn't stop others from reviewing this, particularly if you better understand the intent here.

responds to feedback

In D138939#3963496, @erichkeane wrote:

In D138939#3963473, @tschuett wrote:

Maybe void FormatDiagnostic(SmallVectorImpl<char> &OutStr, DiagnosticMode mode)instead of void FormatDiagnostic(SmallVectorImpl<char> &OutStr)?
To make the transition easer and future proof.

I like this idea. Perhaps with DiagnosticMode being a 3-way enum:

enum struct DiagnosticMode {
  Legacy,
  Sarif,  
  Default = Legacy
}

I like the idea in particular, since it makes a command line flag to modify "Default" to be whichever the user prefers pretty trivial.

though I find myself wondering if the "FormatDiagnostic" call should stay the same, and choose the legacy-reason only when a sarif reason doesn't exist? Or for some level of command line control?

The clang-side interface to this seems a touch clunky, and I fear it'll make continuing its use/generalizing its use less likely.

I don't understand why test_demangle.pass.cpp was considered too big to upload. Here's the diff:

@njames93 if you don't have any further concerns, I am going to merge this on Friday afternoon, as it will have been four months since there has been a maintainer's input.

LGTM. Thanks for fixing this!