[cfe-dev] [PATCH] Docs on how to write RAV based ASTFrontendActions

[Manuel Klimek]

On Fri, Apr 20, 2012 at 4:47 PM, Nikola Smiljanic <popizdeh at gmail.com> wrote:
> Maybe you could mention the libraries needed to link all this?

I added a little snippet for a CMakeLists.txt that compiles it.

On Fri, Apr 20, 2012 at 4:33 PM, Dean Sutherland <dsutherland at cert.org> wrote:
> It's GREAT to see this documentation.  That said, I'll pick a few nits in this one.
>
> First, you need to go on a "which" hunt.  A very simple rule of thumb is that almost all uses of "which" should really be "that" instead. This applies to all instances in the RAVFrontendAction text -- I think.

/me pulls out "BUGS in Writing". Ah, yes. I killed all but 3 whiches
that were wrong, which left two that I think are just introducing
additional information, and one that shows which contexts are
completely different - perhaps that one should be replaced by "what"?
(dumping the AST nodes will show *which | what* nodes are already
being visited)

> "... to implement a RecursiveASTVisitor to extract the information we are interested in from…" should read
> "... to implement a RecursiveASTVisitor to extract the relevant information from…"
> More generally, there are a LOT of "the X we are interested in" in this document.  Most of these would read better as "the relevant X".

I found 2 instances of "interested" in the text; both reworded
according to your suggestion.

> "...; the exception are TypeLoc nodes which are passed by-value."
> This specific "which" should remain!

Really? Wouldn't that then say that all TypeLoc nodes ... Oh, forget
it, I'm stupid. Yep, putting that back in again. Now that leaves me
with 4 whiches. Please let me know if one of them is still incorrect.

> "We can implement just the methods for the nodes we're interested in." might be better written as
> "We only need to implement the methods for the relevant nodes." (or perhaps "… node types.").

Hm. I'm not sure that this is exactly what I meant to say (it sounds
better though, so I'm fine with it :)

> "For example, when we want to find all…" might be better as
> "For example, to find all…"

Done.

> "availale" should be "available"

Done.

> One final style issue.  I find it useful to format names of code entities that appear in running text in the code font to emphasize their "program-ness"; similarly, code snippets in running text work better in the code font. By comparison, concepts like "an AST" should be text font -- even thought the type AST would show up in code font.  It's a bit of a pain to format documents this way, but it does improve clarity.

I want to do more formatting, but later. I'm a launch and iterate guy
:) I think it's much more important to have all the information out
*first*, and then go and make it all pretty, create some coloring
scheme, link everything to the doxygen etc. But, as I said, content
first.

Thx a lot for the review!
/Manuel

> Please feel free to ignore any or all of the above if it violates any project style guidelines, or even if it simply seems like a bad idea to you.
>
> Dean F. Sutherland
> dsutherland at cert.org
>
>
> On Apr 20, 2012, at 8:14 AM, Manuel Klimek wrote:
>
>> As promised, the next step in the tutorial section...
>>
>> Cheers,
>> /Manuel
>> <RAVFrontendAction.html><ATT00001.c>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120420/4d84e0c7/attachment.html>