[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new module and a check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: docs/clang-tidy/checks/openmp-use-default-none.rst:12
+being implicitly determined, and thus forces developer to be explicit about the
+desired data scoping for each variable.
+
----------------
If I understand correctly the issue is more about implicitly shared variables that lead to data-races and bad access patterns, is that correct?
If so it might clarify the reason for this check, if added directly in the first motivational sentence.


================
Comment at: test/clang-tidy/openmp-use-default-none.cpp:67
+  // CHECK-NOTES: :[[@LINE-3]]:9: warning: OpenMP directive 'parallel for' is allowed to contain the 'default' clause, and 'default' clause exists with 'shared' kind. Consider using 'default(none)' clause instead.
+  // CHECK-NOTES: :[[@LINE-4]]:26: note: Existing 'default' clause is specified here.
+}
----------------
lebedev.ri wrote:
> JonasToth wrote:
> > why rewrite the `default(shared)`? I don't exactly understand the reason for not accepting `default(shared)`.
> This check enforces `default(none)`. If the default of `shared` is specified,
> that is still not `none`, therefore it is incorrect and should be diagnosed.
> 
> Why not `shared` but `none`? I tried to cover that in the doc,
> basically it helps prevent issues by forcing one to be explicit about the "visibilities"
> of the variables.
> 
> I suppose it the default can be configurable,
> i'm just not aware of any reason why one would specify `shared`.
I have the honor to work on a originally fortran77 scientific code (which is getting modernized step by step) but there it is very common to have 100 variables with maximum length of 4 characters for the variable names.
One would expect the same quality for similar C-Code when OMP beeing used, so there might be the practical reason that 20 lines of declarations what variable is what are unreadable (because the rest is a big desaster already). But in that case one will not use this check..


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57113