[PATCH] D98657: [flang][driver] Add options for -Werror

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I really like how you split your tests into two files:

- `werror_scan.f` captures warning generated by the prescanner
- `werror.f` captures warnings from the semantic analysis

In every case you added multiple RUN lines to make sure that the behavior is consistent across multiple actions. I think that that's very useful. Ideally, we'd have one central switch for turning warnings into errors and this would be unnecessary. But we're not there yet. In the meantime, could you add a comment explaining **why** multiple RUN lines are used?

I have 2 more suggestions:

- add yet another file for warnings from the parser
- rename `werror.f` as ``werror_sema.f`

These are non-blockers for me.



================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:360
+    res.SetWarnAsErr(true);
+  }
+}
----------------
AFAIK, no other option is allowed in `-W<option>`. Perhaps throw a diagnostic here?


================
Comment at: flang/test/Driver/werror.f90:2
+! Ensure argument -Werror work as expected.
+! TODO: Modify test cases in test/Semantics/ related to f18 when -std= is upstreamed
+
----------------
[nit] This comment is not directly related to what's in this file (i.e. it doesn't really help understand it, does it)? I would remove it. Same in `werror_scan.f`.


================
Comment at: flang/test/Driver/werror.f90:4-6
+!--------------------------
+! FLANG DRIVER (flang-new)
+!--------------------------
----------------
DELETEME


================
Comment at: flang/test/Driver/werror_scan.f:5-7
+!--------------------------
+! FLANG DRIVER (flang-new)
+!--------------------------
----------------
DELETEME


================
Comment at: flang/test/Driver/werror_scan.f:9-11
+!-----------------------------------------
+! FRONTEND FLANG DRIVER (flang-new -fc1)
+!-----------------------------------------
----------------
This is going to be either `flang-new -fc1` or `f18`. I think that we can start skipping this part of the comment to make it more generic.


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

https://reviews.llvm.org/D98657