[-Wdocumentation][RFC] \param with multiple identifiers

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[-Wdocumentation][RFC] \param with multiple identifiers

Kristóf Umann via cfe-dev
I'm working on bug 14335 [1] and have a proof-of-concept patch. This
patch allows a single \param command with multiple identifiers. For
example:

/** Sets the position.
 *  @param [in] x,y,z Coordinates of the position in 3D space.
 *  @param [in] t     The timestamp of the position.
 */
void setPosition(double x, double y, double z, double t);

Generates AST output like:

|-ParagraphComment 0x560955761db0 <line:1291:4, line:1292:4>
| |-TextComment 0x560955761d60 <line:1291:4, col:22> Text=" Sets the position."
| `-TextComment 0x560955761d80 <line:1292:3, col:4> Text="  "
|-ParamCommandComment 0x560955761dd0 <col:5, line:1293:4> [in] explicitly Param="x,y,z"
| `-ParagraphComment 0x560955761ed0 <line:1292:22, line:1293:4>
|   |-TextComment 0x560955761e80 <line:1292:22, col:63> Text=" Coordinates of the position in 3D space. "
|   `-TextComment 0x560955761ea0 <line:1293:3, col:4> Text="  "
`-ParamCommandComment 0x560955761ef0 <col:5, col:52> [in] explicitly Param="t" ParamIndex=3
  `-ParagraphComment 0x560955761f80 <col:18, col:52>
    `-TextComment 0x560955761f50 <col:18, col:52> Text="     The timestamp of the position."

So it works, but I wonder what I should do about the ParamIndex. This
variable is an usigned in ParamCommandComment class.

1.
I can change ParamIndex to a SmallVector and change the AST output to
  [in] explicitly Param="x,y,z" ParamIndex=1 ParamIndex=2 ParamIndex=3
I wonder whether that would make the users of the AST output happy.
Suddenly there can be multiple ParamIndex 'fields'.

2.
I can also add a new magic number MultipleArgParamIndex and add an
isMultipleArgParam function and change the AST output to
  [in] explicitly Param="x,y,z" MultipleArg

3.
Alternatively I change nothing and the `ParamIndex` remains set to
`InvalidParamIndex`. (This is what happens now.)

I personally lean towards option 2, but I'm not sure about the inpact of
these changes of the consumers of the AST output. Note this only changes
the output when multiple arguments are used in a \param command. When
using a single argument is used nothing changes.


[1] https://bugs.llvm.org/show_bug.cgi?id=14335

Kind regards,
Mark de Wever
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [-Wdocumentation][RFC] \param with multiple identifiers

Kristóf Umann via cfe-dev
Hi Mark,

Thank you for working on this bug!

On Sun, Aug 25, 2019 at 4:30 PM Mark de Wever <[hidden email]> wrote:
> 1.
> I can change ParamIndex to a SmallVector and change the AST output to
>   [in] explicitly Param="x,y,z" ParamIndex=1 ParamIndex=2 ParamIndex=3
> I wonder whether that would make the users of the AST output happy.
> Suddenly there can be multiple ParamIndex 'fields'.

I would strongly prefer this option. With regards to APIs, AST or
otherwise, Clang's first priority is to keep code understandable and
maintainable in the long run, backwards compatibility is not a
priority. Therefore, we should choose the API that makes the most
sense, and then adjust the rest of the code as needed.


Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [-Wdocumentation][RFC] \param with multiple identifiers

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev

> 1.
> I can change ParamIndex to a SmallVector and change the AST output to
>   [in] explicitly Param="x,y,z" ParamIndex=1 ParamIndex=2 ParamIndex=3
> I wonder whether that would make the users of the AST output happy.
> Suddenly there can be multiple ParamIndex 'fields'.

Note however that AST classes are allocated with a BumpPtrAllocator, and
that therefore their destructor is not executed. They should therefore be
trivially destructible. This is not currently enforced but I have put two
patches for review (D66646 and D66722) which enforce this with a static_assert.

What you can do instead is either:

1) If you need to have the flexibility to change the size of the array,
   or if 2) is too complicated, just store a pointer to the array and
   allocate the array with the allocator (for an example see
   FunctionDecl::setParams).

2) Or if the number of elements in the array is fixed and known when
   the AST node is created, store them just after the node in memory
   with llvm::TrailingObjects (plenty of examples, just grep for TrailingObjects).
   This is especially easy if the class is a leaf class.

Bruno
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [-Wdocumentation][RFC] \param with multiple identifiers

Kristóf Umann via cfe-dev
On Sun, Aug 25, 2019 at 08:25:31PM +0100, Bruno Ricci wrote:

>
> > 1.
> > I can change ParamIndex to a SmallVector and change the AST output to
> >   [in] explicitly Param="x,y,z" ParamIndex=1 ParamIndex=2 ParamIndex=3
> > I wonder whether that would make the users of the AST output happy.
> > Suddenly there can be multiple ParamIndex 'fields'.
>
> Note however that AST classes are allocated with a BumpPtrAllocator, and
> that therefore their destructor is not executed. They should therefore be
> trivially destructible. This is not currently enforced but I have put two
> patches for review (D66646 and D66722) which enforce this with a static_assert.

You're totally right. I should have written ArrayRef. Thanks for your
patches, I already had a look at D66722.

Cheers,
Mark
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [-Wdocumentation][RFC] \param with multiple identifiers

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
Hi Dmitri,

On Sun, Aug 25, 2019 at 06:58:58PM +0200, Dmitri Gribenko wrote:

> > I can change ParamIndex to a SmallVector and change the AST output to
> >   [in] explicitly Param="x,y,z" ParamIndex=1 ParamIndex=2 ParamIndex=3
> > I wonder whether that would make the users of the AST output happy.
> > Suddenly there can be multiple ParamIndex 'fields'.
>
> I would strongly prefer this option. With regards to APIs, AST or
> otherwise, Clang's first priority is to keep code understandable and
> maintainable in the long run, backwards compatibility is not a
> priority. Therefore, we should choose the API that makes the most
> sense, and then adjust the rest of the code as needed.

Then I'll look at this approach. Thanks for the feedback.

Cheers,
Mark
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev