[PATCH] D93544: [flang][driver] Refactor unit tests for frontend actions (nfc)

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thank you for reviewing @sameeranjoshi !



================
Comment at: flang/unittests/Frontend/FrontendActionTest.cpp:9
 
 #include "gtest/gtest.h"
 #include "flang/Frontend/CompilerInstance.h"
----------------
sameeranjoshi wrote:
> Just trying to understand - why is this error from clang-tidy?
> Also I see https://buildkite.com/llvm-project/premerge-checks/builds/20385#c11feba0-7c79-4290-8631-3026a7ff89aa
> failing for the same.
I've not been able to reproduce this :( To me it looks like the pre-merge script is doing something unexpected. Clearly this line is perfectly fine.

I reported this with [[ https://github.com/google/llvm-premerge-checks/issues/271 | llvm-premerge-checks ]] - hopefully they will have the bandwidth to look into this.


================
Comment at: flang/unittests/Frontend/FrontendActionTest.cpp:72-76
+    // Clear the output files. Note that these tests use an output buffer (as
+    // opposed to an output file), hence there are no physical output files to
+    // delete. Also, some actions don't generated output (e.g.
+    // ParseSyntaxOnly), but it's safe to call this anyway.
+    compInst_.ClearOutputFiles(/*EraseFiles=*/false);
----------------
sameeranjoshi wrote:
> Does it mean that setting `ClearOutputFiles` to `false` will always not delete the files, as you are trying to mention that it's safe to call it anyways ?
> If that's true what happens when a command line option creates physical file?
> So should this be left to individual `TEST_F` function?
CASE 1
In normal circumstances, the compiler's job is to generate output files, so:
* `EraseFiles` will be set to `False` (i.e. the output files should be kept after the compiler exits)
* `ClearOutputFiles` will simply resets the internal state of `CompilerInstance`.

CASE 2
However, in some cases the compiler should delete the files, e.g.:
* when compilation fails: https://github.com/llvm/llvm-project/blob/d6abd7317a269dc7d0204edb8e98f8fcc1a18a2f/flang/lib/Frontend/FrontendAction.cpp#L20-L26
* when the output files are only used for testing (and should be removed once the test is complete).

CASE 3
Finally, some actions don't generate output files, e.g. `ParseSyntaxOnly`, so calling `ClearOutputFiles` doesn't really do much.

In all cases calling `ClearOutputFiles(/*EraseFiles=*/false);` gives the desired behavior.

Let me clarify the comment to make it clearer!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93544/new/

https://reviews.llvm.org/D93544