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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
