[PATCH] D107089: [flang][driver] Add print function name Plugin example

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thank you for addressing my comments @stuartellis ! I've left a couple of comments inline. I would be tempted to further split the test file into multiple files:

- one test file to show that function and subroutine definitions are counted as expected (count should be != 0)
- one test file to show that function/subroutine **calls** are ignored (with count == 0, if possible)
- one test file to show that **interfaces** are not taken into account (with count == 0, if possible)
- one test file with a misspelled plugin name

Also, the tests should probably be located in `flang/test/examples` rather than `flang/test/Driver`.



================
Comment at: flang/examples/PrintFlangFunctionNames/PrintFlangFunctionNames.cpp:67
+  private:
+    bool isInSubprogram{false};
+  };
----------------
> Non-public data members should be named with leading miniscule (lower-case) letters, internal camelCase capitalization, and a trailing underscore, e.g. DoubleEntryBookkeepingSystem myLedger_;.
https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md#naming




================
Comment at: flang/test/Driver/plugin-print-fns.f90:9-13
+!----------------------------------------------
+! Check Error Diagnostic for wrong plugin name
+!----------------------------------------------
+! RUN: not %flang_fc1 -load %llvmshlibdir/flangPrintFunctionNames%pluginext -plugin -wrong-name %s 2>&1 | FileCheck %s --check-prefix=ERROR
+! ERROR: error: unable to find plugin '-wrong-name'
----------------
Could you move this test to a separate file? It's orthogonal to other things being tested here and IMO it makes sense to keep it separate. Basically, I would split the testing as follows:
* test for expected output to verify that the plugin works as expected when _everything is fine_
* test for diagnostics to verify that any abnormal behavior is reported in a sane way.

Otherwise, different tests blend with each other. And by moving them to separate files, you can give the test files a bit more accurate/descriptive names, e.g.:
* `functions_and_subroutines.f90`
* `invalid_plugin_name.f90`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107089