aaron.ballman added subscribers: cjdb, aaron.ballman. 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. ---------------- 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? ================ 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; ---------------- 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? ================ 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< ---------------- 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 ================ Comment at: clang/include/clang/Sema/Lookup.h:121 + /// Name lookup results in an ambiguity because multiple placeholder + /// variables were found in the same scope + /// @code ---------------- ================ Comment at: clang/lib/Sema/SemaDecl.cpp:2171-2172 + if (VD->isPlaceholderVar()) + return; + ---------------- 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. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:7481 + if (!Previous.empty()) { + auto Decl = *Previous.begin(); + const bool sameDC = Decl->getDeclContext()->getRedeclContext()->Equals( ---------------- Please spell out the type here. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:7482 + auto Decl = *Previous.begin(); + const bool sameDC = Decl->getDeclContext()->getRedeclContext()->Equals( + DC->getRedeclContext()); ---------------- ================ Comment at: clang/lib/Sema/SemaDecl.cpp:7484-7486 + if (sameDC && isDeclInScope(Decl, CurContext, S, false)) { + DiagPlaceholderVariableDefinition(D.getIdentifierLoc()); + } ---------------- ================ Comment at: clang/lib/Sema/SemaDecl.cpp:8331 + // Never warn about shadowing placeholder variable + if (ShadowedDecl->isPlaceholderVar()) ---------------- ================ Comment at: clang/lib/Sema/SemaDecl.cpp:18217 + NewFD->setIsPlaceholderVar(LangOpts.CPlusPlus && II && II->isPlaceholder()); if (PrevDecl && !isa<TagDecl>(PrevDecl)) { ---------------- shafik wrote: > cor3ntin wrote: > > shafik wrote: > > > Why can't we fold this into `FieldDecl::Create`? This comment applies in > > > some other places as well. > > By adding a parameter to FieldDecl::Create? We could, I'm not sure it would > > necessarily be cleaner. Let me know what you prefer. > It seems like having the code consolidated make for better maintainability > and figuring this out for future folks but I am willing to be convinced > otherwise. We need to support partial construction because of AST deserialization, but it seems to me that we don't need to add a new parameter to `FieldDecl::Create()` anyway; can't we have the `FieldDecl` constructor look at the given `IdentifierInfo *` in that case, and if it's nonnull, set the placeholder from that? ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:886 DeclarationNameInfo NameInfo(B.Name, B.NameLoc); + IdentifierInfo *VarName = B.Name; LookupResult Previous(*this, NameInfo, LookupOrdinaryName, ---------------- `assert(VarName && "Cannot have an unnamed binding declaration");` ? ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:917 + if (IsPlaceholder) { + const bool sameDC = (Previous.end() - 1) + ->getDeclContext() ---------------- ================ Comment at: clang/lib/Sema/SemaLambda.cpp:1151-1153 + if (Var && Var->isInitCapture() && C->Id->isPlaceholder()) { + Var->setIsPlaceholderVar(true); + } ---------------- ================ 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}} ---------------- I don't understand what this diagnostic is trying to tell me, can you explain a bit more? ================ 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}} ---------------- 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? 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
