[PATCH] D67613: [DWARF-5] Support for DWARF-5 C++ language tags

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX marked 2 inline comments as done.
SouraVX added inline comments.


================
Comment at: include/clang/AST/DeclCXX.h:2931
+    lang_cxx_11 = /* DW_LANG_C_plus_plus_11 */ 0x001a,
+    lang_cxx_14 = /* DW_LANG_C_plus_plus_14 */ 0x0021
   };
----------------
aprantl wrote:
> SouraVX wrote:
> > aprantl wrote:
> > > SouraVX wrote:
> > > > aprantl wrote:
> > > > > I understand that DWARF does not define a C++17 language constant, but in the AST I fell like we should represent it, even if it is being lowered into C++14 for the debug info.
> > > > It's represented in AST as enum in LangStandard.h file. We didn't add it here, as DWARF5 has no opcode for C++17. If we add it here some opcode, this might cause conflict with future upcoming DWARF standard.
> > > Since these are the DWARF DW_lang constants, we shouldn't repeat their numeric values here. Can we write this as
> > > `lang_c = llvm::dwarf::DW_LANG_C`, etc?
> > We can do this, but not sure whether, we should do this in AST ?
> > Since it's a language specific part, if we integrate this from DWARF opcodes it will unnecessary pollute AST ?  Other language opcodes will also be visible here. 
> I'm not sure I understand the concern here. Are you worried about #Including "llvm/BinaryFormat/DWARF.h" and making these definitions visible to users of DeclCXX.h? All symbols there are in a separate namespace.
Thanks @aprantl  for your review.
I was sceptical about,  introducing DWARF related changes to AST, however you're correct, those are just language enums. 
This will be heplfull in future, while adding language tag.  If more C/C++ versions become part of DWARF standard.
Will address this change in next revision.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67613/new/

https://reviews.llvm.org/D67613