ken-matsui added a comment. Thank you so much for your clear review!
================ Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1023-1024 +// Typoed directive warnings +def TypoedDirective : DiagGroup<"typoed-directive">; + ---------------- aaron.ballman wrote: > We don't typically use "typo" in the user-facing part of diagnostics and this > group doesn't seem likely to be reused, so I would remove it (another comment > on this elsewhere). Ah, I see. Thank you! ================ Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:362-365 +def warn_pp_typo_directive : Warning< + "'#%0' directive not found, did you mean '#%1'?">, + InGroup<TypoedDirective>; ---------------- aaron.ballman wrote: > Rather than adding a warning over the top of an existing error, I think we > should modify `err_pp_invalid_directive` to have an optional "did you mean?" > when we find a plausible typo to correct. > > However, we do not issue that diagnostic when it's inside of a skipped > conditional block, and that's what the crux of > https://github.com/llvm/llvm-project/issues/51598 is about. As @rsmith > pointed out in that issue (and I agree), compilers are free to support custom > directives and those will validly appear inside of a conditional block that > is skipped. We need to be careful not to diagnose those kinds of situations > as an error. However, we still want to diagnose when the unknown directive is > "sufficiently close" to another one which can control the conditional chain. > e.g., > ``` > #fi FOO // error unknown directive, did you mean #if? > #endfi // error unknown directive, did you mean #endif? > > #if FOO > #esle // diag: unknown directive, did you mean #else? > #elfi // diag: unknown directive, did you mean #elif? > #elfidef // diag: unknown directive, did you mean #elifdef > #elinfdef // diag: unknown directive, did you mean #elifndef > > #frobble // No diagnostic, not close enough to a conditional directive to > warrant diagnosing > #eerp // No diagnostic, not close enough to a conditional directive to > warrant diagnosing > > #endif > ``` > Today, if `FOO` is defined to a nonzero value, you'll get diagnostics for all > of those, but if `FOO` is not defined or is defined to 0, then there's no > diagnostics. I think we want to consider directives that are *very likely* to > be a typo (edit distance of 1, maybe 2?) for a conditional directive as a > special case. > > Currently, we only diagnose unknown directives as an error. I think for these > special cased conditional directive diagnostics, we'll want to use a warning > rather than an error in this circumstance (just in case it turns out to be a > valid directive in a skipped conditional block). If we do go that route and > make it a warning, I think the warning group should be `-Wunknown-directives` > to mirror `-Wunknown-pragmas`, `-Wunknown-attributes`, etc and it should be > defined to have the same text as the error case. e.g., > ``` > def err_pp_invalid_directive : Error< > "invalid preprocessing directive%select{|; did you mean '#%1'?}0" > >; > def warn_pp_invalid_directive : Warning< > err_pp_invalid_directive.Text>, InGroup<DiagGroup<"unknown-directives">>; > ``` > WDYT? > > (These were my thoughts before seeing the rest of the patch. After reading > the patch, it looks like we have pretty similar ideas here, which is great, > but leaving the comment anyway in case you have further opinions.) > Currently, we only diagnose unknown directives as an error. I think for these > special cased conditional directive diagnostics, we'll want to use a warning > rather than an error in this circumstance (just in case it turns out to be a > valid directive in a skipped conditional block). If we do go that route and > make it a warning, I think the warning group should be `-Wunknown-directives` > to mirror `-Wunknown-pragmas`, `-Wunknown-attributes`, etc and it should be > defined to have the same text as the error case. e.g., > ``` > def err_pp_invalid_directive : Error< > "invalid preprocessing directive%select{|; did you mean '#%1'?}0" > >; > def warn_pp_invalid_directive : Warning< > err_pp_invalid_directive.Text>, InGroup<DiagGroup<"unknown-directives">>; > ``` > WDYT? > > (These were my thoughts before seeing the rest of the patch. After reading > the patch, it looks like we have pretty similar ideas here, which is great, > but leaving the comment anyway in case you have further opinions.) For now, I totally agree with deriving a new warning from `err_pp_invalid_directive`. However, for future scalability, I think it would be great if we could split those diagnostics into Error & Warning and Help, for example. Rustc does split the diagnostics like the following, and I think this is quite clear. So, a bit apart from this patch, I speculate creating a diagnostic system that can split them would bring Clang diagnostics much more readable. https://github.com/rust-lang/rust/blob/598d89bf142823b5d84e2eb0f0f9e418ee966a4b/src/test/ui/suggestions/suggest-trait-items.stderr ================ Comment at: clang/include/clang/Tooling/LevDistance.h:1 +//===--- LevDistance.h ------------------------------------------*- C++ -*-===// +// ---------------- aaron.ballman wrote: > You shouldn't need this file or the implementation file -- we have this > functionality already in: > https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/edit_distance.h > and > https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/StringRef.h#L240. I totally missed the implementations! Sorry. But where should I put both `findSimilarStr` & `findSimilarStr`? It seems that their same implementations aren't there. ================ Comment at: clang/lib/Lex/PPDirectives.cpp:441-449 + const SourceLocation &endLoc) { + const std::array<std::string, 8> candidates{ + "if", "ifdef", "ifndef", "elif", "elifdef", "elifndef", "else", "endif"}; + + if (const auto sugg = + tooling::levdistance::findSimilarStr(candidates, Directive)) { + CharSourceRange DirectiveRange = ---------------- aaron.ballman wrote: > Mostly just cleaning up for coding conventions, but also, no need to use a > `std::array` and we typically don't use local top-level `const` qualification. Thank you! Just wondering, but is there any reason not to use the `const` qualifier? ================ Comment at: clang/test/OpenMP/predefined_macro.c:7 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=50 -verify -o - %s -// expected-no-diagnostics +// expected-warning@+5 {{'#elsif' directive not found, did you mean '#elif'?}} #ifdef FOPENMP ---------------- aaron.ballman wrote: > ken-matsui wrote: > > I am not sure if this typo was intended. > > > > When I renamed `elsif` to `elif`, `#error "_OPENMP has incorrect value"` on > > line `13` was evaluated. > > > > Therefore, if this typo was intended, just suppressing the warning with > > `expected-warning` would be better. However, if this typo was NOT intended, > > I think I should make `_OPENMP` equal to `201107`. It is also possible that > > this test was mistakenly written. > I tracked down the root cause here and fixed the bug in > 50b51b1860acbfb775d5e2eee3310e25c635d667, so when you rebase on main you > won't have to deal with this any longer. Good catch! Thank you for fixing the bug! I could confirm the bug was fixed upstream. ================ Comment at: clang/test/Preprocessor/suggest-typoed-directive.c:10 +// expected-warning@+11 {{'#elfidef' directive not found, did you mean '#elifdef'?}} +// expected-warning@+11 {{'#elfindef' directive not found, did you mean '#elifdef'?}} +// expected-warning@+11 {{'#elsi' directive not found, did you mean '#else'?}} ---------------- aaron.ballman wrote: > It's interesting that this one suggested `#elifdef` instead of `#elifndef` -- > is there anything that can be done for that? > > Also, one somewhat interesting question is whether we want to recommend > `#elifdef` and `#elifndef` outside of C2x/C++2b mode. Those directives only > exist in the latest language standard, but Clang supports them as a > conforming extension in all language modes. Given that this diagnostic is > about typos, I think I'm okay suggesting the directives even in older > language modes. That's as likely to be a correct suggestion as not, IMO. > It's interesting that this one suggested `#elifdef` instead of `#elifndef` -- > is there anything that can be done for that? I found I have to use `std::min_element` instead of `std::max_element` because we are finding the nearest (most minimum distance) string. Meanwhile, `#elfindef` has 2 distance with both `#elifdef` and `#elifndef`. After replacing `std::max_element` with `std::min_element`, I could suggest `#elifndef` from `#elfinndef`. > Also, one somewhat interesting question is whether we want to recommend > `#elifdef` and `#elifndef` outside of C2x/C++2b mode. Those directives only > exist in the latest language standard, but Clang supports them as a > conforming extension in all language modes. Given that this diagnostic is > about typos, I think I'm okay suggesting the directives even in older > language modes. That's as likely to be a correct suggestion as not, IMO. I agree with you because Clang implements those directives, and the suggested code will also be compilable. I personally think only not compilable suggestions should be avoided. (Or, we might place additional info for outside of C2x/C++2b mode like `this is a C2x/C++2b feature but compilable on Clang`?) --- According to the algorithm of `std::min_element`, we only get an iterator of the first element even if there is another element that has the same distance. So, `#elfindef` only suggests `#elifdef` in accordance with the order of `Candidates`, and I don't think it is beautiful to depend on the order of candidates. I would say that we can suggest all the same distance like the following, but I'm not sure this is preferable: ``` #elfindef // diag: unknown directive, did you mean #elifdef or #elifndef? ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124726/new/ https://reviews.llvm.org/D124726 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
