- User Since
- Aug 20 2014, 4:35 PM (404 w, 4 d)
Thu, May 19
libObject uses MemoryBufferRefs which play back and forth with StringRef, and while it has been pointed out over and over again that many of StringRef’s methods should probably be on MemoryBufferRef because they are more broadly useful, it is the way it is.
Wed, May 18
Fixing incorrect code coment.
Tue, May 17
Mon, May 16
Thu, May 12
Tue, May 10
Two small nitpicks, otherwise LGTM.
Mon, May 9
Updating based on PR feedback. Thank's @kuhar!
Two super nitpick points off the top:
(1) Please be consistent that DXIL is all capitalized. I'd like LLVM to not have the annoyance issues that DXC's codebase has where DXIL is inconsistently capitalized.
(2) Can we instead name the pass "DXILTranslateMetadata"? The pass doesn't actually emit the metadata, it is translating it from one form in the module to another.
Sun, May 8
Thu, May 5
Wed, May 4
Tue, May 3
Mon, May 2
LGTM. Thank you for a nice improvement!
The other issue should be fixed in rGb26e44e623c75c084e865084b18541c6a1736df2.
Bots should be fixed with rG55e13a6bc0d6a31afc258a012184253041b0eb8e.
Fri, Apr 29
One more set of updates. Thanks @MaskRay!
Updates based on PR feedback
Thu, Apr 28
Wed, Apr 27
Updates based on PR feedback.
I posted a PR to swift that migrates build-script to use LLVM_ENABLE_RUNTIMES: https://github.com/apple/swift/pull/58465
LGTM! Thank you!
Tue, Apr 26
Making it an ERROR and providing an option to downgrade it to a WARNING seems reasonable to me. Thoughts?
Making the warning message more specific.
I pointed out a few (not all) the places where you have unneeded brackets. Also all your new files don't have newlines at the end of them (the C standard specifies that as a requirement although pretty much all compilers just issue a warning).
I don't want to roadblock or say "no we absolutely can't let this in", _but_... Apple Clang and Swift have long been the only reason we've kept this code alive. I didn't have the time to fix Apple Clang years ago, and nobody has made the effort since.
I question whether we should be extending this or killing it off... Is there a reason you're using LLVM_BUILD_EXTERNAL_COMPILER_RT instead of LLVM_ENABLE_RUNTIMES=compiler-rt?
Rebasing and cleaning up code.
Fixing a code comment.
Updates based on PR feedback.
Mon, Apr 25
This should fix the patch upload, also added in some more of @kuhar's suggestions :).
Ugh... please hold off reviewing. I somehow messed up the commit update and got a lot of noisy diffs.
Updates based on review feedback. Thanks @kuhar!
Comments below. Updated patch coming shortly.
Overall this seems like a nice change. There is one unrelated change fixing a typo in here that should be removed and posted separately.
Sat, Apr 23
Apr 20 2022
This looks good to me and seems like a good added check.
Apr 16 2022
Apr 15 2022
I already merged a fix to resolve the test errors.
@wolfgangp I can't reproduce the failure locally, but I have a guess what's going wrong. I _think_ the issue is that the SmallStrings aren't null terminated and the cleared allocations aren't zero'd. I pushed a speculative fix in rG329abac134a3. Hopefully that fixes it. If not feel free to revert the changes, @python3kgae you'll have to diagnose further issues as I'll be afk all of next week.
Apr 14 2022
@nhaehnle sorry, I realized this patch hadn't been rebased since the request to just fork the full bitcode writer.
Rebasing patches on the updated patch stack.
Rebasing on updated patches from earlier in the stack.
Updates to test cases to increase coverage.
One comment inline. Update coming in a few minutes.
Apr 13 2022
Updating based on PR feedback.