[PATCH] D79484: [DebugInfo] Fortran module DebugInfo support in LLVM

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/include/llvm/IR/DIBuilder.h:746
+    /// \param File        Source file of the module declaration. Used for
+    /// Fortran modules.
+    /// \param LineNo      Source line no. of the  module declaration.
----------------
Nit: `///                    Fortran modules.`


================
Comment at: llvm/include/llvm/IR/DIBuilder.h:748
+    /// \param LineNo      Source line no. of the  module declaration.
+    ///  Used for Fortran modules.
     DIModule *createModule(DIScope *Scope, StringRef Name,
----------------
indentation again


================
Comment at: llvm/include/llvm/IR/DIBuilder.h:751
+                           StringRef ConfigurationMacros, StringRef IncludePath,
+                           StringRef APINotesFile = {}, DIFile *File = {},
+                           unsigned LineNo = 0);
----------------
`= nullptr` is also more obvious


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2081
 
-/// A (clang) module that has been imported by the compile unit.
-///
+/// Represents a module in the programming  language, for example, a Clang
+/// module, or a Fortran module.
----------------
double space after `programming`


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:4831
+///   ::= !DIModule(scope: !0, name: "SomeModule", configMacros:
+///   "-DNDEBUG",includePath: "/usr/include", apinotes: "module.apinotes",
+///   file: !1, line: 4)
----------------
missing space after `,`


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1430
+                         getMDString(Record[4]), getMDString(Record[5]),
+                         getMDString(Record[6]), Record[7])),
         NextMetadataNo);
----------------
This still looks wrong! This will access beyond Record.size() if `Record.size() == 6`.


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:842
            IncludePath == RHS->getRawIncludePath() &&
            APINotesFile == RHS->getRawAPINotesFile();
   }
----------------
Here you do need to compare all fields! It's just not necessary to hash them.


================
Comment at: llvm/unittests/IR/MetadataTest.cpp:2062
   StringRef APINotes = "/tmp/m.apinotes";
+  unsigned LineNo = {};
 
----------------
let's use a real, nonzero number here


================
Comment at: llvm/unittests/IR/MetadataTest.cpp:2085
+                             APINotes, LineNo));
+  EXPECT_NE(N, DIModule::get(Context, File, Scope, Name, ConfigMacro, Includes,
+                             "other", LineNo));
----------------
You need to add two extra tests here. One where only File is different, one where only Line is different.

(We can skip the hashing for performance, but they still must *compare* differently)


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

https://reviews.llvm.org/D79484