[PATCH] D49366: Add support for /FUNCTIONPADMIN command-line option

Zachary Turner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 16 09:31:58 PDT 2018


zturner added inline comments.


================
Comment at: COFF/PDB.cpp:118-122
+  Expected<const CVIndexMap &> mergeDebugT(ObjFile *File,
+                                           CVIndexMap &ObjectIndexMap);
 
-  Expected<const CVIndexMap&> maybeMergeTypeServerPDB(ObjFile *File,
-                                                      TypeServer2Record &TS);
+  Expected<const CVIndexMap &> maybeMergeTypeServerPDB(ObjFile *File,
+                                                       TypeServer2Record &TS);
----------------
stefan_reinalter wrote:
> zturner wrote:
> > It looks like you may have run clang-format against the entire source files, rather than against only the lines that you changed.  Would it be possible to revert these changes and only clang-format changed lines?  There's a script called git-clang-format in the clang source tree.  Usually what we do is add this location to `PATH`, and then run `git clang-format`  That will only format touched lines.
> Yes, I ran clang-format against all touched files. I thought this was the intention to make everything use the same format, but can revert the changes of course.
Usually we try to keep CLs limited to only changes that contribute to the functionality you're implementing in this patch.  clang-formatting an entire file might be worthwile, but we should do it in a separate patch (before or after this patch doesn't matter) that way the only thing we see on the review is changes specifically related to implementing `/FUNCTIONPADMIN`


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D49366





More information about the llvm-commits mailing list