[PATCH] D40408: [LLD] [COFF] Implement numerical sorting of constructors with priority

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: COFF/Writer.cpp:329
 
+static bool orderSections(const StringRef& FullA, const StringRef& FullB) {
+  StringRef A = FullA;
----------------
ruiu wrote:
> ruiu wrote:
> > ruiu wrote:
> > > Please add a comment saying that this is for MinGW. I'd also mention that this could have been technically avoided because we could have used '$' instead of '.', but we do this for compatibility with GNU.
> > Passing a StringRef by reference seems a bit odd. The class is designed to be passed by value, because it is essentially a pointer with size.
> Can you describe the rule that this function is trying to implement briefly?
> Please add a comment saying that this is for MinGW.

Sure

> I'd also mention that this could have been technically avoided because we could have used '$' instead of '.',

No, this still couldn't have been avoided by using '$' instead of '.', since half of the point is to order sections according to the numerical value, not alphabetically. Alphabetically, 123 would be ordered before 5.

> Passing a StringRef by reference seems a bit odd. The class is designed to be passed by value, because it is essentially a pointer with size.

Yes, that's obviously better. I had tried that, but missed that I needed to update the prototype in the declaration of `std::map<..> Map` as well.

> Can you describe the rule that this function is trying to implement briefly?

Order sections alphabetically, as before, with two exceptions:
- If the base section name is followed by a period, it should be sorted before section names followed by a dollar sign. An alphabetical sort would give the order `.ctors`, `.ctors$zzz`, `.ctors.123`.
- If both base section names are followed by a period, order them according to the numerical value following the period, instead of the lexical sort of the strings.

I can update with the redundant `const StringRef&` removed and some comments added.


https://reviews.llvm.org/D40408