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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1430
+                         getMDString(Record[4]), getMDString(Record[5]),
+                         getMDString(Record[6]), Record[7])),
         NextMetadataNo);
----------------
aprantl wrote:
> This still looks wrong! This will access beyond Record.size() if `Record.size() == 6`.
Is this Okay ? if record size is 6 then let the other records null and only access them otherwise.
I didn't noticed any regression failure after this.


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:842
            IncludePath == RHS->getRawIncludePath() &&
            APINotesFile == RHS->getRawAPINotesFile();
   }
----------------
aprantl wrote:
> Here you do need to compare all fields! It's just not necessary to hash them.
Thanks for correcting here. I've added the both fields for comparison.


================
Comment at: llvm/unittests/IR/MetadataTest.cpp:2085
+                             APINotes, LineNo));
+  EXPECT_NE(N, DIModule::get(Context, File, Scope, Name, ConfigMacro, Includes,
+                             "other", LineNo));
----------------
aprantl wrote:
> 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)
Done! Thanks!


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

https://reviews.llvm.org/D79484