aaron.ballman added inline comments.
================ Comment at: clang/docs/ReleaseNotes.rst:135-136 - Compiler flags ``-std=c++2c`` and ``-std=gnu++2c`` have been added for experimental C++2c implementation work. +- Implemented `P2169R4: A nice placeholder with no name <https://wg21.link/P2169R4>`_. This allows to use `_` + as a variable name multiple times in the same scope and is supported in all C++ language modes as an extension. ---------------- cor3ntin wrote: > aaron.ballman wrote: > > Should we consider adding this as an extension to C? It would be conforming > > there as well because it wouldn't change the behavior of any correct C > > code, right? > Maybe we should consult WG14 first? > It's of very questionable usefulness without structured bindings and > destructors. We certainly could ask WG14 for their opinion, but the scenario I was thinking this would help with is for complex macros where variables are sometimes defined but the names are not important as they shouldn't escape. However, I think those scenarios generally introduce different scopes (often with GNU statement expressions) so there's not a naming conflict that we'd need to avoid. So I'm fine skipping C for now until we hit a concrete use case. ================ Comment at: clang/include/clang/AST/DeclBase.h:340-342 + /// Whether this declaration denotes a placeholder that can be + /// redefined in the same scope. + unsigned IsPlaceholder : 1; ---------------- cor3ntin wrote: > aaron.ballman wrote: > > This seems a bit heavy to put on every `Decl`, it only matters for > > declarations with names, right? Should this be on `NamedDecl` instead? And > > should it be using terminology like "name independent"? > > > > Actually, do we need this bit at all? Would it work for `NamedDecl` to have > > a function that returns whether the declaration name is `_` or not? Oh, I > > suppose we can't get away with that because `void _(int x);` does not > > introduce a placeholder identifier but a regular one, right? > I think you made me realize we can probably compute that completely without > using bits. Oups. > It would certainly simplify things. I'll investigate! Thank you! ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6589 +def err_using_placeholder_variable : Error< + "referring to placeholder '_' is not allowed">; +def note_reference_placeholder : Note< ---------------- cor3ntin wrote: > cjdb wrote: > > aaron.ballman wrote: > > > I don't think this helps the user understand what's wrong with their > > > code, especially given the somewhat odd language rules around the > > > feature. How about: `ambiguous reference to multiply-defined placeholder > > > '_'` or something along those lines? Then the note can show the previous > > > declarations of the placeholders that are in scope? e.g., > > > ``` > > > void g() { > > > int _; // note: placeholder declared here > > > _ = 0; > > > int _; // note: placeholder declared here > > > _ = 0; // error: `ambiguous reference to multiply-defined placeholder '_'` > > > } > > > ``` > > > CC @cjdb > > Agreed. I'd suggest a rewording though: I took "multiply" to mean the maths > > term until completing the sentence, rather than its alternative meaning of > > "multiple instances" (which is more or less the same meaning, but > > "multiply" maps to the `x * y` operation for me). > > > > Perhaps `ambiguous reference to placeholder '_', which has multiple > > definitions`? Not sold on that being the best wording, but it does avoid > > the hardcoded-word-at-8yo problem :) > @cjdb I like your suggestion. maybe "which is defined multiple times?" - > pedantically a bunch of things have been defined once each, it's not a > redefinition of the same entity in the c++ sense I like either of your suggestions better than my use of "multiply-defined." :-) ================ Comment at: clang/lib/Sema/SemaDecl.cpp:2171-2172 + if (VD->isPlaceholderVar()) + return; + ---------------- cor3ntin wrote: > aaron.ballman wrote: > > What's the rationale here? I know the standard has a recommended practice, > > but I don't see why that should apply to the case where the user assigns a > > value to the placeholder but never references it otherwise. > The intent is that this behave like `[[maybe_unused]]` Ah, thank you! I think we should make a slightly wider change here then. Let's move `VD->hasAttr<UnusedAttr>()` out of the first if statement and into this one, then add a comment about the relationship between the attribute and this feautre. WDYT? ================ Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:31 + // expected-note 4{{placeholder declared here}} \\ + // expected-warning 2{{placeholder variable has no side effect}} + (void)_++; // expected-error {{referring to placeholder '_' is not allowed}} ---------------- cor3ntin wrote: > aaron.ballman wrote: > > I don't understand what this diagnostic is trying to tell me, can you > > explain a bit more? > The no side effect one? > You are assigning something that has no side effect to something that you may > never be able to name, so the whole thing is likely dead code. > I welcome improvements suggestions Yeah, the no side effect one. Corentin and I discussed this off-list and I understand the situation a bit better now. Basically, an init capture named `_` either has a side effect on construction and/or destruction (like an RAII object) or it cannot be used within the lambda. However, it does still have effects (for example, it impacts whether the lambda has a function conversion operator). That makes this kind of tricky to word. Given that it's a QoI warning, I'd recommend we leave this bit for a follow-up so that we can land the majority of the functionality and we can add the safety rails later. ================ Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:79 +void test_param(int _) {} +void test_params(int _, int _); // expected-error {{redefinition of parameter '_'}} \ + // expected-note {{previous declaration is here}} ---------------- cor3ntin wrote: > aaron.ballman wrote: > > Weird.. is this expected? You can have two variables named `_` in block > > scope, but you can't have two parameters named `_` despite them inhabiting > > the same block scope? > Yes, CWG decided against having it in parameter declarations, because > otherwise we'd need them in template parameters which we didn't want to do. > And it would be useless, you can simply not name them. I guess I see the same amount of utility in allowing an init capture named `_` as I do a function parameter named `_` as a local variable named `_` (if it is an RAII type, it can do useful things in theory; otherwise, it's not a useful declaration). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153536/new/ https://reviews.llvm.org/D153536 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
