[CPP-387] Create a new PR against new location of introduce-libraries…

[zlaski-semmle]

…-cpp.rst.

Previous PR was https://git.semmle.com/Semmle/code/pull/33238

[geoffw0]

What's the simplest way for me to see what this will look like when it's rendered?

How complete is this PR at the moment?

[zlaski-semmle]

I'd say it is 80% done. Formatting is totally inconsistent and still a bit in the air as far as what conventions should be used (feedback welcome). I've pasted the link to the file below.

[zlaski-semmle]

You can preview the file at https://github.com/zlaski-semmle/ql/blob/zlaski/cpp387/docs/ql-documentation/learn-ql/cpp/introduce-libraries-cpp.rst

[zlaski-semmle]

So I have committed the first, very rough draft of the document. It will be much easier for you to look at the rendered document (https://github.com/zlaski-semmle/ql/blob/zlaski/cpp387/docs/ql-documentation/learn-ql/cpp/introduce-libraries-cpp.rst) instead of analyzing the diffs.

@james, @jbj, @dave-bartolomeo, @rdmarsh2 @geoffw0 , please give me feedback regarding formatting (esp. wrt the C/C++ code snippets), structure, and which QL class entries should be removed or are missing.

[geoffw0]

A few thoughts.

[geoffw0]

Why are certain words such as "enum_constant1" and (later) "strcat" bold?

[zlaski-semmle]

Because the formatting has still not been formalized. Which of the various "styles" contained in the document are preferable to you?

[geoffw0]

I think I prefer when the declaration syntax is all in a monospaced font, with no bold or italics. Bolding the element in question (as in strcat) also looks reasonable to me. Consistency is important though.

[jf205]

Be aware that unfortunately rst does not support nested inline mark up. So, for example, you can't have text that is both monospace and bold.

[jbj]

As we discussed in the Hangout, I'd like to see all the named functions (inet_addr, memcpy, etc.) removed as well as all the named builtins (__is_nothrow_assignable etc.). This is for the sake of maintainability and to make sure this becomes an overview page rather than a reference page. There's already a reference at https://help.semmle.com/qldoc/cpp/.

[jf205]

This requires rebasing to fix the conflicts, as the ql/docs/ql-documentation directory has been renamed ql/docs/language. The sphinx previews should be automatically generated after the rebase (artifacts of the Doc/Generate-Sphinx PR job).

[jbj]

Why do you write class C { instead of class C {?

[jbj]

These are my comments after a first pass of going over this in some detail. I think another pass will be needed, both by yourself and by someone else.

But first let's figure out the formatting. Since @jf205 points out that we can't have bold monospace text, I'm withdrawing my opinion that the element in focus should be bold-face. It looks strange to have a mix of monospace and variable-width text. The inspiration all along has been JavaScript's overview page, and I still find JavaScript's overview page to pleasant to read.

I think now that the reason why the examples are so complicated is that you've attempted to make an injective mapping from QL class to surface syntax, whereas I think it would be more helpful as a map from surface syntax to QL classes. For example, I'd have just one entry for a - b, and its QL class column would have both SubExpr, PointerDiffExpr, and PointerSubExpr. This is the overview page, and it should be helpful to users who know C++ but don't know the QL C++ library.

Before you iterate more on this, let's discuss it in a Hangout to make sure we're on the same page.

[jbj]

This + should be ++.

[zlaski-semmle]

Where?

[jbj]

Why not write this as nullptr_t?

[zlaski-semmle]

Can you point me at the rendered document? I'm not sure what you're referring to.

[jbj]

IntPointerType is an example of a type that's far too specific and ad hoc to be documented here. I suggest we remove all items from the semmle/code/cpp/commons directory. What do you think, @geoffw0 ?

[geoffw0]

Perhaps FormattingFunction / FormattingFunctionCall are useful enough to be worth documenting here, but with that possible exception I agree about commons.

[jbj]

GNU extension

[zlaski-semmle]

Where?

[jbj]

GNU extension

[zlaski-semmle]

Where?

[jbj]

I don't think it's helpful to mention ptrdiff_t here, especially not in bold-face. It's not obvious that the relevant item in this example is the - and not something else.

[jbj]

I can see that GitHub is also confused about which lines my review comments should be on. It looks correct on the Files tab.

[jbj]

@zlaski-semmle My comments are no longer visible on the Files tab after the latest pushes, but this url still shows all my comments in the right place.

[zlaski-semmle]

I removed the WIP tag, so please feel free to review and/or merge this.

[jbj]

I haven't finished reviewing, but this is what I have so far.

• PrefixIncrExpr still has just + instead of ++.
• As per Geoffrey's comment, please remove everything from commons except FormattingFunction (replacing AttributeFormattingFunction and UserDefinedFormattingFunction) and FormattingFunctionCall.
• For C :: C (void);: We don't write (void) in C++, just ().
• I find the FriendDecl example overly long and strangely formatted. Please just keep the two lines with friend in them (minus the };).
• How are the lines supposed to be ordered? I think something could be done with the ordering to kepp related concepts together.

[zlaski-semmle]

The GitHub rST compiler does seem to format things strangely. Here is what I'm seeing locally, as built by Sphinx v1.8.5 and rendered by Microsoft Edge:

[zlaski-semmle]

I've gotten rid of all the "commons" QL classes, but kept FormatLiteral in addition to FormattingFunction and FormattingFunctionCall.

[zlaski-semmle]

@jbj, I'm puzzled by your constructor suggestion. People definitely write void, as it is a documenting feature.

[jbj]

People definitely write void, as it is a documenting feature.

Okay. Then let's keep it that way, as long as it's deliberate.

[jbj]

Okay, I finally went through the whole thing from top to bottom.

• ConversionOperator: I believe the syntax is just "operator T ();" with no arguments, as a member function. There seems to be no way to declare such conversions as non-member functions.
• TopLevelFunction, VirtualBaseClass: Not canonical enough to be included in the overview.
• I haven't found the pattern for when you write "Type" (as a link) and when you write a placeholder type like int or float. I prefer "Type", but it's not important enough to block the PR.
• Constructor: replace void with "...".
• ConversionConstructor: I think it's confusing to write class D instead of just D.
  VirtualFunction: Missing (.
• It varies whether members are written as class C { ... } or as C::...;. Compare the example for Constructor to the example for NoArgConstructor -- they're the same but written in these two different styles. I personally prefer the C::...; style since it focuses better on the AST element in question and is shorter.
• Our hierarchy of Class, Struct, and Union is not what most people expect, and this overview page could help reduce confusion. You can link to Class from both class C { … }, struct S { … } and union U { … }, and you can link to Struct from both struct S { … } and union U { … }.
• Class, Struct, Union: I think the members in their examples are more confusing than they are helpful. Is it not a Class unless it has a member function?
• ProxyClass, FoldExpr, ParenthesizedBracedInitializerList: you can write in the "Remarks" column that these elements only appear in uninstantiated templates.
• FriendDecl: stray backtick
• AsmStmt: Missing double quote.
• FunctionTryStmt: Stray } at the end.
• ArrayExpr: The example is really "Expr [ Expr ]" since the array part can be an arbitrary pointer-typed expression. Similar for DeleteArrayExpr and DeleteExpr.
• VariableCall: Isn't that more commonly written as "Expr(Expr)", without the *? For example, syscall_table[index](arg1, arg2, arg3);.
• FoldExpr: missing ; before }.
• LambdaExpression: I think the example would be clearer if you leave out the float a parameter so it's instead implied that a is captured. Then the lambda is not just a function pointer.
• FormatLiteral: Should %sn be %s\n? Maybe the backslash got lost.
• ParenthesizedBracedInitializerList: Are you sure your example is the right syntax? The toString on the class returns ({ … }), so maybe just go with that.

[zlaski-semmle]

I'll try responding on a per-bullet-point basis.

• You're right; I just assumed that conversion operators can be static member functions (or global function) like the other operators, but this is not the case. I'll change the example to reflect this.
• I can replace TopLevelFunction with Function (which we do not have yet!), but what of VirtualBaseClass? It models virtual inheritance, which is an important C++ construct. Let me know how you'd like me to proceed here.
• I don't really have a pattern here. Since these are all examples, I thought it wise to mix abstract and concrete syntax. Do you think we should have just one or the other? Or perhaps you have reservations about particular examples? Please let me know.
• Done.
• Since we are dealing with example code, I inserted these variations on purpose, for better syntactic "coverage". But let me know if you prefer the C:: notation throughout.
• Done.
• I'm not sure I understand your question here; classes, structs and unions can all have member functions. The examples for the three are a tad repetitive, but I couldn't think of anything better.
• Done.
• Done.
• Done.
• Done.
• Done (although the concrete-versus-abstract syntax discussion could have been had here).
• I used * to explicitly show that we were calling via indirection. You're right in that * is usually omitted (though not in the code I write). But I like your syscall_table idea better.
• Done.
• Done (though I renamed a to captured to indicate what was going on).
• Done.
• Done. Note about uninstantiated templates was removed since the new examples do not use templates.

[jbj]

I can replace TopLevelFunction with Function (which we do not have yet!), but what of VirtualBaseClass? It models virtual inheritance, which is an important C++ construct. Let me know how you'd like me to proceed here.

The problem with VirtualBaseClass is that it describes how the class is used elsewhere in the snapshot. So the same class may be a VirtualBaseClass in one snapshot and an ordinary Class in another snapshot even though it hasn't changed at all.

I agree we need to represent class inheritance on the overview page. For that, you need to add the ClassDerivation class. There's no VirtualClassDerivation class, only a ClassDerivation.isVirtual predicate, so virtual inheritance wont't be directly represented on the overview page.

I don't really have a pattern here. Since these are all examples, I thought it wise to mix abstract and concrete syntax. Do you think we should have just one or the other?

I don't think there's value in varying the meta-syntax used in the examples. I think "Type" (as a link) would be most consistent with the rest of the page and with the JavaScript page. But don't change it now if it's a hassle.

I'm not sure I understand your question here; classes, structs and unions can all have member functions.

Sorry, my question was rhetorical and meant as an example of what a reader might ask themselves. My suggestion is to simply omit the members and write class C { ... }; and so on (with literal ...). But if you think it's clearer to include the members, I'm fine with that.

... But I like your syscall_table idea better.

I only noticed just now that you've documented a class named VariableCall, which is a particular case of ExprCall. I don't think VariableCall is interesting or canonical, so please replace it with ExprCall. Then it also becomes correct to write "Expr ( Expr )".

Note about uninstantiated templates was removed since the new examples do not use templates.

Are you sure the examples you wrote will actually produce a ParenthesizedBracedInitializerList in QL? I'd be fine with dropping this class entirely from the overview. It's not used anywhere in this repo or the internal repo.

[jbj]

Since we just found out that Function, ExprCall and ClassDerivation were missing from the overview, I wonder whether other important classes are missing. Did you have a systematic way to find candidates for which elements should be included in the overview? My best suggestion would be to look through https://lgtm.com/query/2990471420024879007/.

[jbj]

There's no VirtualClassDerivation class

It turns out there is a VirtualClassDerivation class.

[jbj]

My best suggestion would be to look through https://lgtm.com/query/2990471420024879007/.

To be clear, I'm not advocating that all these classes should be on the overview page. I'm just proposing it as a way to check whether we might have missed one or two classes.

The DeclarationEntry hierarchy of classes isn't part of the overview, and I think it should stay that way. I think it's also fine to omit Location and File since these aren't really AST elements.

[zlaski-semmle]

I did have a methodology for choosing which QL classes to document: starting with Decl, Stmt, Expr and Preprocessor, I culled the leaves from the respective trees. Hence, Function was omitted as it is an inner node (as is ExprCall). ClassDerivation and VirtualClassDerivation were not considered at all. Since this is an introduction and not a reference, I did not concern myself with traversing the entire Locatable tree.

For purposes of our overview, I guess I'll treat ClassDerivation and VirtualClassDerivation as declarations.

[zlaski-semmle]

When constructing the doc for ParenthesizedBracedInitializerList, I looked here: https://en.cppreference.com/w/cpp/language/list_initialization. I did not see any reference to instantiations therein (and, indeed, there is the global function f in the example that does not require instantiation), and so removed the remark about uninstantiated templates.

Looking back at the QLDoc, it seems that ParenthesizedBracedInitializerList does have something to do with templates. I concocted the following example

template <typename T>
class A: T {
  A(void): T({ 1, 2, 3}) {
    T::f({ 4, 5, 6 });
  }
};

which compiles fine with GCC and Clang, but PrintAST.qll does not produce ParenthesizedBuiltinInitializerLists anywhere. So I'm stumped. I think I will remove ParenthesizedBuiltinInitializerList for the time being.

[jbj]

LGTM. Thank you for putting up with all these iterations of review!

[jbj]

@jf205: I've merged this PR. What's the next step in getting it deployed?

[jf205]

@jbj and @zlaski-semmle: thanks for getting this merged. The next step is to manually publish this on help.semmle.com. There are a couple more PRs that I would like to get merged before doing this, so I expect it will happen towards the end of the week, if that is ok for you?

[jbj]

That's perfectly fine.

[zlaski-semmle]

This week is fine. Please let me know what you need me to do.