Hello,<br><br>Little ping on this :)<br><br>I've joined two "fresher" patches, the only real update is that thanks to Takumi's help I was able to actually run the test I had wrote and thus ensure it passed and tested the functionality as expected.<br>
<br>I am still waiting for comments on:<br>1. the option name, and the fact it is activated by default<br>2. the quality of implementation itself (I hope it did get the naming convention right)<br>3. the standardization of the names<br>
4. whether the display suits you or not<br>5. whether the test suits you or not<br>6. if this seems sufficient for a first patch :)<br><br>I'll forge ahead once this first step is settled.<br><br>Matthieu.<br><br><div class="gmail_quote">
2011/3/19 Matthieu Monrocq <span dir="ltr"><<a href="mailto:matthieu.monrocq@gmail.com">matthieu.monrocq@gmail.com</a>></span><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hello,<br><br>I have further modified the diagnostics system of clang.<br><br>1. I have introduce two new options -fdiagnostics-show-name and -fno-diagnostics-show-name to activate/deactive printing the diagnostic name. It is activated by default (like -fdiagnostics-show-option), I don't suppose there's any opposition, but just in case you wish a different name, shout!<br>
<br>(note: I've copied it in several places, following -fdiagnostics-show-option example, I don't know if I missed something... and I hope I didn't make a typo in one site)<br><br>2. I have added an Index to map the name of the diagnostic back to the ID. For efficient search this index is sorted... on first use. I could not find a way to get the TableGen backend to sort it by itself because the diagnostics are splitted over multiple files. Because it is sorted only on first use, I don't expect any impact (apart from the memory it takes) during a regular run, since it should only be invoked through the help menu. Also, even if sorting is "relatively" expensive (< 2k elements), since it's for the help, I guess it does not matter too much.<br>
<br>I just have one little issue: what if I don't find it ? At the moment I hijacked diag::DIAG_UPPER_LIMIT for signaling an invalid name. Is this correct ?<br><br>I was also thinking that we could probably propose a fuzzy match for a best effort search, in case we didn't locate the name, because users may make a typo when invoking the help. I would suggest adding a boolean parameter to indicate whether the call wishes for this extended search or not. Thoughts ?<br>
<br>3. I have had some troubles extracting names from the IDs. Most IDs are well-behaved and begin by `fatal_`, `err_`, `ext_`, `warn_` or `note_` but there's a couple that do not (beginning by `error_` or embedding the `warn` in the middle of the string or just not indicating anything of the sort). For those, I simply print the whole ID as the name, however I'd like to provide a clean-up (separate) patch to "standardize" them.<br>
<br>4. I have wired it all in the TextDiagnosticPrinter.cpp (as you'll see). At the moment I print it in a separate bracket block<br><br><div style="margin-left: 40px;">$ clang++ error.cpp<br>error.cpp:1:5: error: second parameter of 'main' (argument array) must be of type 'char **' [main_arg_wrong]<br>
int main(int, char*) {<br> ^<br>1 error generated.<br></div><br><div style="margin-left: 40px;">$ clang++ -Wunused-parameter warning.cpp<br>warning.cpp:1:21: warning: unused parameter 'argv' [unused_parameter] [-Wunused-parameter]<br>
int main(int, char* argv[]) { // expected-warning{{unused parameter 'argv'}}<br> ^<br>1 warning generated.<br></div><br>Would you rather I put it in the already existing bracket block ?<br><br>
I've bypassed it for notes, I don't expect the name to be useful for notes, but I would not mind printing it if some felt strongly about it.<br>
<br>I could also bypass it if there is no BriefExplanation, but (at least for warning) we could always display even then the categories it belongs to, the different level of options etc... thoughts ?<br><br>5. I have written a simple test in the FrontEnd section, but I have some trouble with using lit.py on MSYS (MinGW shell), I'll send a separate mail to the list for support on this. I've included the test anyway, to see if the intent is right at least.<br>
<br>6. I was planning on producing the patches in several drops, this being the first. If I get the test to play nice... Does this seem reasonable ? Do you think this patch misses something (apart from a test for the name index... since I'll require to tweak the help for that).<br>
<br><br>And obviously, comments on the patches themselves would be welcome.<br><br>Regards :)<div><div></div><div class="h5"><br><br><div class="gmail_quote">2011/3/14 Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span><br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><br><div><div><div>On Mar 11, 2011, at 12:12 PM, Matthieu Monrocq wrote:</div>
<br><blockquote type="cite">Hello,<br><br>This is the first patch-set to better document clang diagnostics.<br><br>> I have added two fields into the Diagnostic (.td) class: Brief and Explanation; and modified the subsequent generation (and exploitation) of the table<br>
> I have added two getters to DiagnosticIDs to get the "BriefExplanation" and "FullExplanation"<br><br>> There is also a few corrections of comments that were (it seems) out of sync<br><br>Note: there is one (little) patch for LLVM, because the TableGen backend is in llvm repository... and the two patches are intrisically tied (obviously)<br>
<br>Note 2: is this normally tested or do we assume that is compilation a test in itself ?<br></blockquote><div><br></div></div>Compilation, plus any tests for the feature that the TableGen changes support, are enough.</div>
<div><div><br><blockquote type="cite">Unless we want the same Brief / Explanation for Groups too, this normally implement the "libclang" part, so next are the HTML pages generation and the Help menu modification.<br>
</blockquote><div><br></div></div><div>I don't think we need to worry about this just yet.</div><div><br><blockquote type="cite">
For those tasks, the distinction of where the diagnostic comes from
probably does not makes sense for the user, so I would expect to have to
group the warnings using the categories and subgroups rather than AST/Sema/Parse.</blockquote><div><br></div></div><div>Yes. More categorization of the existing diagnostics would be useful.</div><div><br><blockquote type="cite">
I suppose that
we'll want to expose both warnings and extension warnings here.<br></blockquote><div><br></div></div><div>Yes.</div><div><div><br></div><blockquote type="cite">
There is one catch though: the individual warnings are not named. I was
wondering if I should create a name (#ENUM in the macro ? The internal ID ?) or if the Brief would be enough and I'd just
list all the briefs/explanations underneath the subgroup they belong to.<br></blockquote><div><br></div></div><div>Well, the warnings do have the names they're given in the .td files, e.g.,</div><div><br></div><div><div style="margin: 0px;">
def err_array_init_not_init_list : Error<</div><div style="margin: 0px;"> "array initializer must be an initializer "</div><div style="margin: 0px;"> "list%select{| or string literal}0">;</div>
<div><br></div><div>We could take the diagnostic name without the err_/ext_/warn_ prefix, so the name of this diagnostic would be "array_init_not_init_list". TableGen could expose this as the diagnostic name.</div>
</div><div><br><blockquote type="cite"><br>With regard to the HTML generation: I was thinking about using a Python script and invoke it as part of the build step. This would require, I guess, that I update the Python Bindings (if any ?) to extract the information from libclang. I'd appreciate pointers to the documentation (if any).<br>
</blockquote></div></div><div><br></div><div>Well, that's certainly one approach. You'd need to extend libclang to provide diagnostic documentation information, and update the Python bindings for libclang. I guess the benefit is that it's easier to emit HTML from a Python script than from C++.</div>
<div><br></div><div>Personally, I think you should start simpler: add a flag that teaches the diagnostic printer to emit the name of the diagnostic as part of the brackets, e.g.,</div><div><br></div><div><span style="white-space: pre-wrap;"> </span>warning: semicolon before method body is ignored [-Wsemicolon-before-method-body, semicolon_before_method_body]</div>
<div><br></div><div>and then add some kind of --help option that looks up the documentation for a diagnostic based on that name, e.g.,</div><div><br></div><div><span style="white-space: pre-wrap;"> </span>clang --help semicolon_before_method_body</div>
<div><br></div><div>With this working, you'll be able to write regression tests that ensure that we're getting the right documentation for the right names. And users could use this feature to get more information about a particular diagnostic. That entire system could be in place before you start worrying about the formatting of HTML itself, so other people could help fill in content.</div>
<br><div><span style="white-space: pre-wrap;"> </span>- Doug</div></div></blockquote></div><br>
</div></div></blockquote></div><br>