- User Since
- Apr 28 2020, 4:57 AM (25 w, 3 d)
Thu, Oct 22
Wed, Oct 21
Thank you all for you comments! Please find my replies below. I've picked 4 main points raised here.
Tue, Oct 20
Hi @oneraynyday ! This looks very interesting - thanks for uploading! I've only quickly skimmed through. Two high level points:
- Have you considered sending an RFC to cfe-dev regarding this tool? I think that it would be a great way of attracting peoples attention. More importantly, should we add another tool to clang-tools-extra?
- Tthis is a rather large patch - could you split it into separate chunks? Otherwise it's quite tricky to review.
Mon, Oct 19
Apply fix to unit test that got lost in the previous patch, simplify C input file
Address PR comments, make clearer separation between option types, simplify/fix tests
Simplify the API for creating output files
Fri, Oct 16
Rebase + refector the unit test
Thu, Oct 15
I've added more reviewers for the Clang side of this patch. I choose people who most recently changed the functions/files that this patch modifies. Any input much appreciated! For more context regarding Clang changes: http://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html
Address PR comments, clang-format, rebase
Thank you for reviewing @SouraVX! I'm just about to submit an updated patch with the requested changes.
Thanks for testing so extensively! LGTM!
Thank you for fixing this so quickly @serge-sans-paille , that's much appreciated!
Wed, Oct 14
The CMake error for flang-new is now fixed, but I'm no longer able to build Flang with BUILD_SHARED_LIBS=On. @serge-sans-paille , have you tested that configuration? I'm struggling to find a fix.
Revert accidental and unnecessary change
@serge-sans-paille Thank you for working on this! Sadly the Flang buildbot is unhappy again:http://lab.llvm.org:8014/#/builders/109/builds/35. I'm guessing that you didn't test with -DFLANG_BUILD_NEW_DRIVER=ON?
Mon, Oct 12
@reviewers A note regarding the changes in Clang.
Fri, Oct 9
Hi @serge-sans-paille, thanks for working on this!
Thu, Oct 8
Wed, Oct 7
I didn't see any failures on the llvm buildbots that email me when they fail. The other patch will be merged as soon as accepted. Should be tomorrow morning.
Tue, Oct 6
Mon, Oct 5
LGTM, thanks for working on this!
Sat, Oct 3
@tskeith Thank you for the heads up. Updated accordingly and merged!
Fri, Oct 2
/home/flang/temp/llvm-project/flang/lib/Lower/OpenACC.cpp:102:32: error: unused variable 'argTy' [-Werror,-Wunused-variable] llvm::ArrayRef<mlir::Type> argTy; ^ 1 error generated.
Thu, Oct 1
@CarolineConcatto thank you again for working on this! The structure is good, but IMHO this patch could be polished a bit more. Overall:
- could you make sure that this patch does not change the output from clang -help?
- doxygen comments are consistent
- unittests are a bit better documentated (what and why is tested?)
- use llvm/Support/FileSystem.h instead of <filesystem>
More comments inline. Otherwise, I think that this is almost ready :)
Tue, Sep 29
https://reviews.llvm.org/D88351 has caused our Flang buildbots to start failing:
These buildbots are not in production yet (still connected to the staging master), so no notifications are sent.
Mon, Sep 28
Address the remaining PR comments, fix test
Fri, Sep 25
Thu, Sep 24
Rebase on top of master
Sep 24 2020
@sameeranjoshi Apologies, I missed some of your comments.
Sep 23 2020
Thank you for reviewing! I think that I've addressed all your comments.. Please see the updated patch.
Move code from Fortran to Fortran::frontend namespace, address PR comments
Sep 22 2020
@CarolineConcatto , thank you for this patch! It implements some really important functionality and IMO the overall structure is solid.
Sep 21 2020
Thanks for reviewing @sanwou01 ! No new comments, so I'll submit as is.