This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Add checks for errors from `Prescan` and `Parse`
ClosedPublic

Authored by awarzynski on Dec 22 2020, 8:23 AM.

Details

Summary

If either Prescan or Parse generate any fatal errors, the new driver
will:

  • report it (i.e. issue an error diagnostic)
  • exit early
  • return non-zero exit code

This behaviour is consistent with f18 (i.e. the old driver).

Diff Detail

Event Timeline

awarzynski created this revision.Dec 22 2020, 8:23 AM
awarzynski requested review of this revision.Dec 22 2020, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2020, 8:23 AM
sameeranjoshi accepted this revision.Jan 4 2021, 10:33 PM

Mostly nit comments.
Thanks and LGMT.

flang/lib/Frontend/FrontendAction.cpp
59

nit - would be better to use const as much as possible if there are not modifications to the variables.

point 13
https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#c-language

60

nit: - Should the error messages start with a capital letter?
https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#error-messages

63

nit - ci ?

This revision is now accepted and ready to land.Jan 4 2021, 10:33 PM
This revision was landed with ongoing or failed builds.Jan 6 2021, 2:20 AM
This revision was automatically updated to reflect the committed changes.
awarzynski marked 2 inline comments as done.

I can see how these messages can be useful for a compiler developer; however, perhaps these messages are redundant for end users because other error messages have been issued.

flang/lib/Frontend/FrontendAction.cpp
60

Good point. The current implementation is consistent with f18: https://github.com/llvm/llvm-project/blob/main/flang/tools/f18/f18.cpp#L212 and that's what I've been prioritizing. I'm not sure whether f18 is intentionally inconsistent with the coding guidelines or not.

I suggest that these messages are updated in a separate patch for both f18 and flang-new. Since it's an NFC, I pushed it without a separate review (https://github.com/llvm/llvm-project/commit/fa1e543e0b8c625bf2625598d9a16c484e349884). Thanks for pointing this out!

I can see how these messages can be useful for a compiler developer; however, perhaps these messages are redundant for end users because other error messages have been issued.

Thank you for the suggestion! I actually pushed this change pretty much at the point when you were sending this. Obviously we can refine this.

For now I'd like to prioritise consistency with f18. Once flang-new is ready to replace f18, we could focus on refining the user experience. Also, when end-users report erros to developers, it's good to have a message that allows developers narrow the error down. So I still think that it's worthwhile to have specialized error messages (e.g. could not parse vs could not scan) in the driver. Lastly, wouldn't the frontend driver be considered more of a _developer_ rather than _end-user_ tool?