Skip to content
Snippets Groups Projects
Commit 2fecf228 authored by Giovanni Bussi's avatar Giovanni Bussi
Browse files

Significant improvements in plumedcheck

- Vim syntax for the awk file
- Some information is given once per file now.
- Registration of vessels and metrics is also logged.
- Check that .h files do not include:
  * inc files (they are temporary)
  *  external .h file (they might be not available when using plumed library).
The latter is only notified now, since it must be accepted for mpi.h
- Separately store documented actions/cltools/other.
- Check that module type is correct.
- Extensive doc for each check added.
parent 88cf56c8
No related branches found
No related tags found
No related merge requests found
#! /bin/bash #! /bin/bash
# vim: ft=awk
TEMP=$(mktemp) TEMP=$(mktemp)
...@@ -99,6 +100,22 @@ BEGIN{ ...@@ -99,6 +100,22 @@ BEGIN{
deprecated_headers["stdlib.h"]=1 deprecated_headers["stdlib.h"]=1
deprecated_headers["string.h"]=1 deprecated_headers["string.h"]=1
deprecated_headers["tgmath.h"]=1 deprecated_headers["tgmath.h"]=1
# list of allowed module types
allowed_module_types["always"]=1
allowed_module_types["default-on"]=1
allowed_module_types["default-off"]=1
# list of "outer modules", that is those
# that are not included in the kernel library
outer_modules["wrapper"]=1
outer_modules["main"]=1
# list of "core modules", that is those for which
# a specific module namespace is not required
core_modules["core"]=1
core_modules["tools"]=1
core_modules["reference"]=1
} }
# for each input file # for each input file
...@@ -131,6 +148,11 @@ BEGINFILE{ ...@@ -131,6 +148,11 @@ BEGINFILE{
if((getline < path > 0) ){ if((getline < path > 0) ){
module_type[module]=$0 module_type[module]=$0
information("module_type","module " module " has type '" module_type[module] "'" ); information("module_type","module " module " has type '" module_type[module] "'" );
# DOC: :wrong_module_type:
# DOC: Module type as indicated in the `module.type` file located in the module directory is not valid.
# DOC: The admitted types are: `default-on`, which means the module is on by default, `default-off`, which means that
# DOC: they should be explicitly enabled, and `always`, which means they are always required.
if(!(module_type[module] in allowed_module_types)) error("wrong_module_type","module " module " has a wrong module type '" module_type[module] "'");
} }
} }
} }
...@@ -159,6 +181,12 @@ BEGINFILE{ ...@@ -159,6 +181,12 @@ BEGINFILE{
# detect copyright lines # detect copyright lines
if($0=="/* +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++") in_copyright=1; if($0=="/* +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++") in_copyright=1;
# check direct inclusion of headers that would bypass module dependencies # check direct inclusion of headers that would bypass module dependencies
# DOC: :include_dots:
# DOC: It is considered an error to include a file using a path starting with `..`.
# DOC: The reason is that this would allow to include PLUMED modules that have not been
# DOC: explicitly declared in the Makefile (e.g. using `#include "../tools/Vector.h"`).
# DOC: This might result in undetected dependences between the modules, which means that
# DOC: by enabling some set of modules one would result in a non-linkable library.
if(match($0,"^# *include +\\\"\\.\\.")) error("include_dots","cannot include files using .. path"); if(match($0,"^# *include +\\\"\\.\\.")) error("include_dots","cannot include files using .. path");
# check if there is anything behind copyright and preprocessor commands # check if there is anything behind copyright and preprocessor commands
...@@ -177,12 +205,39 @@ BEGINFILE{ ...@@ -177,12 +205,39 @@ BEGINFILE{
# } # }
# } # }
# check if deprecated headers are included # check #include <file>
if(match($0,"^# *include +<[^>]+>.*$")){ if(!in_doxygen && match($0,"^# *include +<[^>]+>.*$")){
h=$0 h=$0
sub("^# *include +<","",h) sub("^# *include +<","",h)
sub(">.*$","",h) sub(">.*$","",h)
# check if deprecated headers are included
# DOC: :deprecated_headers:
# DOC: There is a list of headers that have been deprecated in C++ and should not be
# DOC: included. These headers should be replaced with their equivalent from the stdlib.
# DOC: E.g., use `#include <cstdlib>` instead of `#include <stdlib.h>`. Notice that
# DOC: using the modern header all the functions are declared inside the `std` namespace.
if(h in deprecated_headers) error("deprecated_header","including deprecated header " h); if(h in deprecated_headers) error("deprecated_header","including deprecated header " h);
# DOC: :external_header: (style)
# DOC: Header files should possibly not include other header files from external libraries.
# DOC: Indeed, once PLUMED is installed, if paths are not set correctly the include files of the other libraries
# DOC: might not be reachable anymore. This is only a stylistic warning now. Notice that
# DOC: with the current implementation of class Communicator it is necessary to include
# DOC: `mpi.h` in a header file.
if(filetype=="header" && h~"\\.h") style("external_header","including external header " h " in a header file");
}
# check #include "file"
if(!in_doxygen && match($0,"^# *include +\"[^\"]+\".*$")){
h=$0
sub("^# *include +\"","",h)
sub("\".*$","",h)
# check if a .inc file, which is temporary and not installed, is included in a header file.
# this might create inconsistencies in the installed plumed header files.
# DOC: :non_h_header:
# DOC: Files with `.inc` extension are generated by PLUMED while it is compiled and are
# DOC: not installed. They should not be directly included in header files, otherwise
# DOC: those header files could be not usable once PLUMED is installed.
if(filetype=="header" && h!~"\\.h") error("non_h_header","including non '.h' file " h " in a header file");
} }
# check if namespace PLMD is present # check if namespace PLMD is present
...@@ -197,7 +252,9 @@ BEGINFILE{ ...@@ -197,7 +252,9 @@ BEGINFILE{
sub("^ *PLUMED_REGISTER_ACTION\\(.*, *[\"]","",action); sub("^ *PLUMED_REGISTER_ACTION\\(.*, *[\"]","",action);
sub("[\"].*$","",action); sub("[\"].*$","",action);
information("registered_action","action " action); information("registered_action","action " action);
if(action in registered_actions) error("multi_registered","action " action " already registered at "registered_actions[action]); # DOC: :multi_registered_action:
# DOC: Each action should be registered (with `PLUMED_REGISTER_ACTION`) once and only once.
if(action in registered_actions) error("multi_registered_action","action " action " already registered at "registered_actions[action]);
registered_actions[action]=FILENAME ":" FNR; registered_actions[action]=FILENAME ":" FNR;
} }
...@@ -207,16 +264,57 @@ BEGINFILE{ ...@@ -207,16 +264,57 @@ BEGINFILE{
sub("^ *PLUMED_REGISTER_CLTOOL\\(.*, *[\"]","",cltool); sub("^ *PLUMED_REGISTER_CLTOOL\\(.*, *[\"]","",cltool);
sub("[\"].*$","",cltool); sub("[\"].*$","",cltool);
information("registered_cltool","cltool " cltool); information("registered_cltool","cltool " cltool);
if(cltool in registered_cltools) error("multi_registered","cltool " cltool " already registered at "registered_cltools[cltool]); # DOC: :multi_registered_cltool:
# DOC: Each cltool should be registered (with `PLUMED_REGISTER_CLTOOL`) once and only once.
if(cltool in registered_cltools) error("multi_registered_cltool","cltool " cltool " already registered at "registered_cltools[cltool]);
registered_cltools[cltool]=FILENAME ":" FNR; registered_cltools[cltool]=FILENAME ":" FNR;
} }
# take note of registered vessels
if(match($0,"^ *PLUMED_REGISTER_VESSEL")){
vessel=$0
sub("^ *PLUMED_REGISTER_VESSEL\\(.*, *[\"]","",vessel);
sub("[\"].*$","",vessel);
information("registered_vessel","vessel " vessel);
# DOC: :multi_registered_vessel:
# DOC: Each vessel should be registered (with `PLUMED_REGISTER_VESSEL`) once and only once.
if(vessel in registered_vessels) error("multi_registered_vessel","vessel " vessel " already registered at "registered_vessels[vessel]);
registered_vessels[vessel]=FILENAME ":" FNR;
}
# take note of registered metrics
if(match($0,"^ *PLUMED_REGISTER_METRIC")){
metric=$0
sub("^ *PLUMED_REGISTER_METRIC\\(.*, *[\"]","",metric);
sub("[\"].*$","",metric);
information("registered_metric","metric " metric);
# DOC: :multi_registered_metric:
# DOC: Each metric should be registered (with `PLUMED_REGISTER_METRIC`) once and only once.
if(metric in registered_metrics) error("multi_registered_metric","metric " metric " already registered at "registered_metrics[metric]);
registered_metrics[metric]=FILENAME ":" FNR;
}
# take note of documented actions or cltools # take note of documented actions or cltools
if(match($0,"^//[+]PLUMEDOC ")){ if(match($0,"^//[+]PLUMEDOC ")){
doc=$NF; doc=$NF;
information("documented","doc " doc); # DOC: :multi_doc:
# DOC: An action or cltool should be documented once and only once. Notice that because of the
# DOC: way the manual is generated, a cltool cannot have the same name of an action.
if(doc in plumed_doc) error("multi_doc","doc " doc " already at "plumed_doc[action]); if(doc in plumed_doc) error("multi_doc","doc " doc " already at "plumed_doc[action]);
plumed_doc[doc]=FILENAME ":" FNR; plumed_doc[doc]=FILENAME ":" FNR;
switch($(NF-1)){
case "TOOLS":
plumed_doc_cltools[doc]=FILENAME ":" FNR;
information("documented_cltool","doc " doc);
break;
case "INTERNAL":
plumed_doc_internal[doc]=FILENAME ":" FNR;
information("documented_internal","doc " doc);
break;
default:
plumed_doc_action[doc]=FILENAME ":" FNR;
information("documented_action","doc " doc);
}
} }
# check if, besides the \par Examples text, there is a verbatim command # check if, besides the \par Examples text, there is a verbatim command
...@@ -235,6 +333,12 @@ BEGINFILE{ ...@@ -235,6 +333,12 @@ BEGINFILE{
if(match($0,guard)) found_guard_ifdef=1 if(match($0,guard)) found_guard_ifdef=1
# check that "using namespace" statements are not used in header files # check that "using namespace" statements are not used in header files
# DOC: :using_namespace_in_header:
# DOC: A statement `using namespace` in header files is considered bad practice. Indeed, this would imply that
# DOC: any other file including that header file would be using the same namespace.
# DOC: This is true also for `std` namespace, and that's why you typically find full specifications
# DOC: such as `std::string` in header files. In source files you are free to use `using namespace PLMD` and/or
# DOC: `using namespace std` if you like, since these commands will only affect the source file where they are used.
if(filetype=="header" && match($0,"using *namespace") && !in_plumed_doc && !in_doxygen) error("using_namespace_in_header","using namespace statement in header file") if(filetype=="header" && match($0,"using *namespace") && !in_plumed_doc && !in_doxygen) error("using_namespace_in_header","using namespace statement in header file")
} }
...@@ -244,11 +348,20 @@ BEGINFILE{ ...@@ -244,11 +348,20 @@ BEGINFILE{
sub("__PLUMED_HAS_","",string); sub("__PLUMED_HAS_","",string);
if(filetype=="autoconf"){ if(filetype=="autoconf"){
# take note of defined HAS macros # take note of defined HAS macros
information("defined_has",string); if(plumed_definehas_file[string,FILENAME]!="used") information("defined_has",string);
plumed_definehas_file[string,FILENAME]="used";
plumed_definehas[string]=FILENAME ":" FNR; plumed_definehas[string]=FILENAME ":" FNR;
} else { } else {
# take note of used HAS macros # take note of used HAS macros
information("used_has",string); if(plumed_usehas_file[string,FILENAME]!="used") information("used_has",string);
# DOC: :used_has_in_header: (style)
# DOC: Using `__PLUMED_HAS_SOMETHING` macros in headers should be avoided since
# DOC: it could make it difficult to make sure the same macros are correctly defined
# DOC: when using PLUMED as a library. This is only a stylistic warning now. Notice that
# DOC: with the current implementation of class Communicator it is necessary to include
# DOC: `mpi.h` in a header file and thus to use `__PLUMED_HAS_MPI`.
if(plumed_usehas_file[string,FILENAME]!="used" && filetype=="header") style("used_has_in_header",string);
plumed_usehas_file[string,FILENAME]="used";
plumed_usehas[string]=FILENAME ":" FNR; plumed_usehas[string]=FILENAME ":" FNR;
} }
} }
...@@ -267,21 +380,28 @@ BEGINFILE{ ...@@ -267,21 +380,28 @@ BEGINFILE{
# at the end of each file: # at the end of each file:
ENDFILE{ ENDFILE{
# check for namespaces # check for namespaces
# this is done only for non-trivial files (i.e. we skip files that just include an header) if(found_non_preprocessor_lines && (filetype=="source" || filetype=="header") && !(module in outer_modules) ){
# we also skip special modules wrapper and main here, which do not define PLMD namespace # DOC: :missing_namespace_plmd:
if(found_non_preprocessor_lines && (filetype=="source" || filetype=="header") && module!="wrapper" && module!="main" ){ # DOC: Every source file should contain a `PLMD` namespace.
# DOC: Notice that files in the "outer modules" `wrapper` and `main`, that is those that
# DOC: are not included in the kernel library, are exempted from this rule.
if(!found_namespace_plmd) error("missing_namespace_plmd","missing PLMD namespace"); if(!found_namespace_plmd) error("missing_namespace_plmd","missing PLMD namespace");
# files belonging to "core modules" are not supposed to have a module namespace # DOC: :missing_namespace_module:
if(module!="core" && module!="tools" && module!="reference"){ # DOC: Every source file should contain a sub namespace with the same name of the module itself.
# this can be done only if module name is known: # DOC: Notice that there are important exceptions to this rule:
if(module && !found_namespace_module) error("missing_namespace_module","missing " module " namespace") # DOC: - "outer modules" `wrapper` and `main`, that are not included in the kernel library.
} # DOC: - "core modules" that are part of the core of PLUMED. Since the classes implemented here are
# DOC: widely used within PLUMED, they are directly defined in the PLMD namespace.
if(module && !(module in core_modules) && !found_namespace_module) error("missing_namespace_module","missing " module " namespace")
} }
# check if header guard is present
# check is done only if module is known since module name is used to build the macro name
if(filetype=="header"){ if(filetype=="header"){
# this can be done only if module name is known: # this can be done only if module name is known:
# DOC: :missing_guard:
# DOC: Every header file should contain a proper guard to avoid double inclusion.
# DOC: The format of the guard is fixed and should be `__PLUMED_modulename_filename_h`, where
# DOC: `modulename` is the name of the module and `filename` is the name of the file, without suffix.
# DOC: This choice makes guards unique within the whole codebase.
if(module && !found_guard_ifdef) error("missing_guard","missing ifndef guard"); if(module && !found_guard_ifdef) error("missing_guard","missing ifndef guard");
} }
...@@ -297,11 +417,18 @@ ENDFILE{ ...@@ -297,11 +417,18 @@ ENDFILE{
if((getline < cppfile < 0)) notfound=1 if((getline < cppfile < 0)) notfound=1
} }
if(notfound){ if(notfound){
# DOC: :non_existing_cpp:
# DOC: For every header file there should exist a corresponding source file with the same name.
# DOC: Notice that dummy headers that only include another header or which only define preprocessor
# DOC: macros are exempted from this rule.
error("non_existing_cpp","file " file " is a header but there is no corresponding source"); error("non_existing_cpp","file " file " is a header but there is no corresponding source");
} }
while((getline < cppfile)>=0){ while((getline < cppfile)>=0){
if(match($0,"^ *#include")){ if(match($0,"^ *#include")){
sub("^ *#include",""); sub("^ *#include","");
# DOC: :non_included_h:
# DOC: The source file corresponding to a header file (that is: with the same name) should include it as the first included file.
# DOC: This is to make sure that all the header files that we install can be individually included and compiled.
if($1!="\"" file "\"") error("non_included_h","file " file " is a header but " cppfile " does not include it as first include"); if($1!="\"" file "\"") error("non_included_h","file " file " is a header but " cppfile " does not include it as first include");
break; break;
} }
...@@ -318,39 +445,71 @@ END{ ...@@ -318,39 +445,71 @@ END{
# this is required to properly report messages # this is required to properly report messages
global_check=1 global_check=1
# check that all registered actions were documented
for(action in registered_actions){ for(action in registered_actions){
if(!(action in plumed_doc)){ if(!(action in plumed_doc_action)){
# DOC: :undocumented_action:
# DOC: Every action that is registered with `PLUMED_REGISTER_ACTION` should also be documented
# DOC: in a PLUMEDOC page.
error("undocumented_action","action " action " at " registered_actions[action] " is not documented") error("undocumented_action","action " action " at " registered_actions[action] " is not documented")
} else if(!(action in provide_examples)){ } else if(!(action in provide_examples)){
error("action_without_examples","action " action " at " plumed_doc[action] " has no example") # DOC: :action_without_examples:
# DOC: Every action that is registered should contain an `Example` section in its documentation.
# DOC: Notice that if the `Example` section is not added the manual will not show the registered keywords.
error("action_without_examples","action " action " at " plumed_doc_action[action] " has no example")
} else if(!(action in provide_verbatim)){ } else if(!(action in provide_verbatim)){
# this should be made into an error: # DOC: :action_without_verbatim:
# DOC: Every action that is registered should contain an example in the form of a `verbatim` section.
# DOC: This is currently a stylistic warning since there are many actions not satisfying this requirement.
style("action_without_verbatim","action " action " at " provide_examples[action] " has no verbatim") style("action_without_verbatim","action " action " at " provide_examples[action] " has no verbatim")
} }
} }
# check that all registered cltools were documented for(action in plumed_doc_action){
if(!(action in registered_actions))
# DOC: :unregistered_action:
# DOC: Every action that is documented should be also registered using `PLUMED_REGISTER_ACTION`.
error("unregistered_action"," action " action " at " plumed_doc_action[action] " is not registered");
}
for(cltool in registered_cltools){ for(cltool in registered_cltools){
if(!(cltool in plumed_doc)){ if(!(cltool in plumed_doc_cltools)){
# DOC: :undocumented_cltool:
# DOC: Every cltool that is registered with `PLUMED_REGISTER_CLTOOL` should also be documented
# DOC: in a PLUMEDOC page.
error("undocumented_cltool","cltool " cltool " at " registered_cltools[cltool] " is not documented") error("undocumented_cltool","cltool " cltool " at " registered_cltools[cltool] " is not documented")
} else if(!(cltool in provide_examples)){ } else if(!(cltool in provide_examples)){
error("cltool_without_examples","cltool " cltool " at " plumed_doc[cltool] " has no example") # DOC: :cltool_without_examples:
# DOC: Every cltool that is registered should contain an `Example` section in its documentation.
# DOC: Notice that if the `Example` section is not added the manual will not show the registered options.
error("cltool_without_examples","cltool " cltool " at " plumed_doc_cltools[cltool] " has no example")
} else if(!(cltool in provide_verbatim)){ } else if(!(cltool in provide_verbatim)){
# DOC: :cltool_without_verbatim:
# DOC: Every cltool that is registered should contain an example in the form of a `verbatim` section.
error("cltool_without_verbatim","cltool " cltool " at " provide_examples[cltool] " has no verbatim") error("cltool_without_verbatim","cltool " cltool " at " provide_examples[cltool] " has no verbatim")
} }
} }
# check that all used __PLUMED_HAS macros have been defined in autoconf for(cltool in plumed_doc_cltools){
if(!(cltool in registered_cltools))
# DOC: :unregistered_cltool:
# DOC: Every cltool that is documented should be also registered using `PLUMED_REGISTER_CLTOOL`.
error("unregistered_cltool"," cltool " cltool " at " plumed_doc_cltools[cltool] " is not registered");
}
for(has in plumed_usehas){ for(has in plumed_usehas){
if(!(has in plumed_definehas)){ if(!(has in plumed_definehas)){
# DOC: :undefined_has:
# DOC: Every macro in the form `__PLUMED_HAS_SOMETHING` that is used in the code
# DOC: should be defined in configure.ac. This check is made to avoid errors with mis-typed macros.
error("undefined_has","has " has " at " plumed_usehas[has] " is not defined") error("undefined_has","has " has " at " plumed_usehas[has] " is not defined")
} }
} }
# check that all defined __PLUMED_HAS macros have been used in source code
for(has in plumed_definehas){ for(has in plumed_definehas){
if(!(has in plumed_usehas)){ if(!(has in plumed_usehas)){
# DOC: :unused_has:
# DOC: Every macro in the form `__PLUMED_HAS_SOMETHING` that is defined in configure.ac
# DOC: should be used at least once in the code. This check is made to avoid errors with mis-typed macros.
error("unused_has","has " has " at " plumed_definehas[has] " is not used") error("unused_has","has " has " at " plumed_definehas[has] " is not used")
} }
} }
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment