[llvm][discussion] Burn down LLVM patches

[pjreiniger]

I tried to make the LLVM upgrade as non-invasive as possible, which resulted in 31 (some large) patches. Some of these are important quality of life ones and compiler ones, but I think there might be potential to remove some of them. If nothing else, an explanation on why some of these things are happening would be good to talk about

• [ ] ~0004-Change-uniqe_function-storage-size.patch Why the change storage size change? #1761 just mentions that there was an update, but I'm curious the reasoning~
• [x] 0006-Remove-DJB-hash-dependency.patch This removes the need to pull in another header (which uses a different algorithm), but might be worth rolling back?
• [ ] 0007-ifdef-guard-safety.patch I assume the reason for this is in the event that a user also includes a "real" copy of llvm. Would it make more sense to globally replace LLVM_ with WPI_?
• [x] 0011-Remove-allocator-from-collections.patch Is there a big change in overhead using an allocator? The concept is very much a part of the standard library. This tends to be a super annoying patch during an upgrade because it affects every line with the template args on it. The removal of including the AllocatorBase.h header also forces a lot of the updates in the "fixup includes" patch
• [x] 0012-Remove-EpochTracker.patch DenseMap still uses this, why can't the others? When not compiling with NDEBUG it should pretty much get optimized away
• [x] 0010-Remove-reverse-iterator.patch I don't really know what the purpose of this is, but I do know it tends to be an annoying thing to patch on an upgrade
• [ ] 0016-Add-vectors-to-raw_ostream.patch Maybe specialize these things in a separate header and not include in the (eventually moved) thirdparty/llvm area?
• [ ] 0020-Add-lerp-and-sgn.patch I found no usages of lerp, and sgn feels like more of a wpimath thing
• [ ] 0002-Remove-StringRef-ArrayRef-and-Optional.patch As I mentioned in my PR, I did these conversions by hand because it wasn't a simple StringRef -> string_view, `ArrayRef -> span. Maybe it can / should be? This is also an annoying patch between updates.

[pjreiniger]

Also would like to un-snowflake MemoryBuffer and StringExtras, but that is another issue

[calcmogul]

wpi::sgn() gets used a bunch in wpimath and wpilibc. wpi::Lerp() is a shim for C++20's std::lerp().

[PeterJohnson]

0004: The libuv wrappers and uses of them benefit from having a bit more space for captures to avoid memory allocations... I think I actually did some testing on this to decide on 4 vs 3. 0006: Seems reasonable 0007: Correct; a global replace of LLVM with WPI should work just fine 0011: The AllocatorBase.h dependencies look pretty big to me? It's not just AllocatorBase.h but the stuff that it includes... 0012: Yeah this is probably fine to pull out, we probably imported it before DenseMap 0010: Probably fine too 0016: Some of this patch can't be separated, it's directly modifying part of the raw_ostream class. And the upstream one has svector, so it feels like it belongs here. 0002: I don't know how easy it would be do this automatically (particularly since string_view doesn't have all the functions of StringRef, making it necessary to use StringExtras for some things), but this is an important QOL change and we absolutely will not go back to StringRef/ArrayRef.

A lot of StringExtras is the way it is for a reason, in that string_view doesn't have the same functions as StringRef. I would be interested in seeing how you would approach doing this as a patch?

Feel free to take a stab at MemoryBuffer; I think you'll find the patch will end up being a sizeable part of the file.

[pjreiniger]

wpi::sgn() gets used a bunch in wpimath and wpilibc. wpi::Lerp() is a shim for C++20's std::lerp().

<numbers> is a c++20 thing and is implemented in wpimath, these both seem very much like math utils instead of generic utils

[pjreiniger]

Another one I forgot to mention was 0003-Wrap-std-min-max-calls-in-parens-for-windows-warning.patch

Is there a reason that NOMINMAX is not being defined globally? Its the default for bazels msvc toolchain params, and it removes the need to do this wonky (std::min)(x) stuff. I got everything compiling with that macro being set in bazel, doesn't seem like it causes any problems

[calcmogul]

Seems like something we should do.

[PeterJohnson]

Theoretically NOMINMAX could break other libraries teams pull in.

[ThadHouse]

For the big example, anything including gdiplus.h is broken if NOMIXMAX is defined.

[calcmogul]

Libraries still use GDI+?

[pjreiniger]

Even if they do, which is really the exception and which is the rule? I reckon the usages of std::min are far greater than the usages of gdi.

Having a team do the #undef dance if they are actually using it seems to make more sense than wrapping anything that ends in min and max in parens