beanz added a comment.
A few nitpicks, otherwise this looks good.
================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:61
-static void cleanModuleFlags(Module &M) {
- constexpr StringLiteral DeadKeys[] = {ValVerKey};
- // Collect DeadKeys in ModuleFlags.
- StringSet<> DeadKeySet;
- for (auto &Key : DeadKeys) {
- if (M.getModuleFlag(Key))
- DeadKeySet.insert(Key);
- }
- if (DeadKeySet.empty())
- return;
-
- SmallVector<Module::ModuleFlagEntry, 8> ModuleFlags;
- M.getModuleFlagsMetadata(ModuleFlags);
- NamedMDNode *MDFlags = M.getModuleFlagsMetadata();
- MDFlags->eraseFromParent();
- // Add ModuleFlag which not dead.
- for (auto &Flag : ModuleFlags) {
- StringRef Key = Flag.Key->getString();
- if (DeadKeySet.contains(Key))
- continue;
- M.addModuleFlag(Flag.Behavior, Key, Flag.Val);
- }
-}
-
-static void cleanModule(Module &M) { cleanModuleFlags(M); }
+static void cleanModule(Module &M) {}
----------------
Better to remove this function if it is empty. If we need it in the future we
can re-introduce it.
================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:80
bool DXILTranslateMetadata::runOnModule(Module &M) {
- if (MDNode *ValVerMD = cast_or_null<MDNode>(M.getModuleFlag(ValVerKey))) {
- auto ValVer = loadDXILValidatorVersion(ValVerMD);
+ if (auto *ValVerMD = M.getNamedMetadata(ValVerKey)) {
+ auto ValVer = loadDXILValidatorVersion(ValVerMD->getOperand(0));
----------------
nit: LLVM's coding conventions discourage blanket use of `auto`. This one is a
bit on the edge of the convention. My preference would be for this to have the
type because I think that is more clear.
see:
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:81
+ if (auto *ValVerMD = M.getNamedMetadata(ValVerKey)) {
+ auto ValVer = loadDXILValidatorVersion(ValVerMD->getOperand(0));
if (!ValVer.empty())
----------------
This should definitely have the type, not auto.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130207/new/
https://reviews.llvm.org/D130207
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
