[PATCH] D14687: Macro support in LLVM IR

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Thanks for the comments.
Please, see my answers below.


================
Comment at: docs/LangRef.rst:3696
@@ -3696,2 +3695,3 @@
+unit, regardless of code optimizations (some nodes are only emitted if there are
 references to them from instructions).
 
----------------
> I assume these line endings are an artifact of the diff, and won't be committed?
If you mean these lines, then they will be committed, actually, this was in the rst file before my changes.
But, I think it will not be shown in the HTML LangRef document.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2207
@@ -2207,1 +2206,3 @@
+              getMDOrNull(Record[13]), Record.size() <= 14 ? 0 : Record[14],
+              Record.size() <= 15 ? 0 : getMDOrNull(Record[15])),
           NextMDValueNo++);
----------------
> The canonical place for `macros:` should really be *before* `dwoId`.
> It's one of the set of arrays.  Same probably makes sense for the C++ APIs.  The most important thing is LangRef and AsmWriter.  Please move it!
I tried that approach at first. However, it will not work, as it will break the backward compatibility.
There are some LIT tests that have 'dwoId' as record number 14, if I change the order these bitcode files will fail to load using the new bitcode reader.

Do you think that I should not care about backward compatibility and change the order back such that 'macros' appear before 'dwoId'?




Repository:
  rL LLVM

http://reviews.llvm.org/D14687