- 
                Notifications
    
You must be signed in to change notification settings  - Fork 15.1k
 
[clang-doc] integrate JSON as the source for Mustache templates #149589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-doc] integrate JSON as the source for Mustache templates #149589
Conversation
| 
          
 @llvm/pr-subscribers-clang-tools-extra Author: Erick Velez (evelez7) ChangesThis patch integrates JSON as the source to generate HTML Mustache templates. The Mustache generator calls the JSON generator and reads JSON files on the disk to produce HTML serially. Patch is 48.74 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/149589.diff 9 Files Affected: 
  | 
    
b36a2d4    to
    c9f121a      
    Compare
  
    c9f121a    to
    2476174      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall seems like a nice set of changes, and lots of removed code is always nice to see. I left a couple Q's inline, but once those are addressed, this is probably good to land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if ((EC = File.getError())) | |
| continue; | |
| if (EC = File.getError(); EC) | |
| continue; | 
But, independent of the code style, why is this continue? If EC is an error, then on the next iteration you'll report an error due to failing to iterate the directory, but this looks like a different error kind to me. What's the logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, my original intent was to reuse the error reporting at the beginning of the loop but that doesn't work because of the message. The error can just return here.
Another thought reading this though, should the entire generation fail if one file fails? Would it better to report the file failing and then continue with the generation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. You could buffer the error reporting in a global vector and report at the end. There's probably a better way to issue diagnostics if we hooked into the diagnostics reporting mechanisms in clang or llvm. That's a bit involved though, so maybe just do something simple (like log a warning for now?) and file an issue upstream under clang-doc to incorporate proper diagnostics reporting via the diagnostics engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once you're satisfied the diagnostics are in order.
2476174    to
    bc443f0      
    Compare
  
    25f5252    to
    a2851dd      
    Compare
  
    bc443f0    to
    7ecaad5      
    Compare
  
    …#149589) This patch integrates JSON as the source to generate HTML Mustache templates. The Mustache generator calls the JSON generator and reads JSON files on the disk to produce HTML serially.

This patch integrates JSON as the source to generate HTML Mustache templates. The Mustache generator calls the JSON generator and reads JSON files on the disk to produce HTML serially.