aprantl added a comment. Thank you this looks very good! Just a few questions inline.
================ Comment at: llvm/lib/AsmParser/LLParser.cpp:4171 +template <> +bool LLParser::ParseMDField(LocTy Loc, StringRef Name, DIBTFlagField &Result) { + ---------------- Chirag wrote: > Chirag wrote: > > Chirag wrote: > > > aprantl wrote: > > > > Could the bulk of the implementation of this function be shared with > > > > the function that parses DIFlag and DISPFlag? > > > Does using macro to generate flag LLParser seems like a good idea? it can > > > be reused for other flags as well. > > something like, > > > > #define FLAG_FIELDS(MDNODE, FLAG_NAME) > > \ > > struct DI##FLAG_NAME##Field : public MDFieldImpl<MDNODE::DI##FLAG_NAME##s> > > { \ > > DI##FLAG_NAME##Field() : MDFieldImpl(MDNODE::FLAG_NAME##Zero) {} > > \ > > }; > > > > FLAG_FIELDS(DINode, Flag) > > FLAG_FIELDS(DIBasicType, BTFlag) > > FLAG_FIELDS(DISubprogram, SPFlag) > > > > > > #define FLAG_FIELDS_PARSER(MDNODE, FLAG_NAME) > > \ > > template <> > > \ > > bool LLParser::ParseMDField(LocTy Loc, StringRef Name, > > \ > > DI##FLAG_NAME##Field &Result) { > > \ > > > > \ > > auto parseFlag = [&](MDNODE::DI##FLAG_NAME##s &Val) { > > \ > > if (Lex.getKind() == lltok::APSInt && !Lex.getAPSIntVal().isSigned()) { > > \ > > uint32_t TempVal = static_cast<uint32_t>(Val); > > \ > > bool Res = ParseUInt32(TempVal); > > \ > > Val = static_cast<MDNODE::DI##FLAG_NAME##s>(TempVal); > > \ > > return Res; > > \ > > } > > \ > > > > \ > > if (Lex.getKind() != lltok::DI##FLAG_NAME) > > \ > > return TokError("expected debug info flag"); > > \ > > > > \ > > Val = MDNODE::getFlag(Lex.getStrVal()); > > \ > > if (!Val) > > \ > > return TokError(Twine("invalid basicType debug info flag '") + > > \ > > Lex.getStrVal() + "'"); > > \ > > Lex.Lex(); > > \ > > return false; > > \ > > }; > > \ > > > > \ > > MDNODE::DI##FLAG_NAME##s Combined = MDNODE::FLAG_NAME##Zero; > > \ > > do { > > \ > > MDNODE::DI##FLAG_NAME##s Val; > > \ > > if (parseFlag(Val)) > > \ > > return true; > > \ > > Combined |= Val; > > \ > > } while (EatIfPresent(lltok::bar)); > > \ > > > > \ > > Result.assign(Combined); > > \ > > return false; > > \ > > } > > > > FLAG_FIELDS_PARSER(DINode, Flag) > > FLAG_FIELDS_PARSER(DIBasicType, BTFlag) > > FLAG_FIELDS_PARSER(DISubprogram, SPFlag) > > > > > i will create a reusable function for flag parsing. Sharing the parsing code was not feasible? ================ Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1515 + // Some flags were moved from DIFlags to SPFlags + DISubprogram::moveDItoSPFlags(Flags, SPFlags); + ---------------- I believe this should only be called conditionally? Or specifically: What will happen when we reuse flag bits that are in the range of the old now freed-up DIFlags? ================ Comment at: llvm/lib/IR/AsmWriter.cpp:1721 +void MDFieldPrinter::printDIBTFlags(StringRef Name, + DIBasicType::DIBTFlags Flags) { ---------------- Again, can this function share code with the printDISPFlags function? ================ Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:366 + switch (Flag) { + // Appease a warning. +#define HANDLE_DIBT_FLAG(ID, NAME) \ ---------------- Which warning is being appeased here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74470/new/ https://reviews.llvm.org/D74470 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits