cleanup doxygen for IceClFlags.h

cleanup doxygen for IceClFlags.h

BUG=
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=cb0d47b67fea89acba4dc14dc9ce19ee3f12132b

[rkotlerimgtec, 2015-12-16 04:24:23]

Description was changed from

==========
cleanup doxygen for IceClFlags.h

BUG=
==========

to

==========
cleanup doxygen for IceClFlags.h

BUG=
==========

[rkotlerimgtec, 2015-12-16 04:24:23]

rkotlerimgtec@gmail.com changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org

[rkotlerimgtec, 2015-12-16 04:25:56]

Here is a cut at making the IceClFlags.h very readable in the documentation and
where the IceClFlags class members are used elsewhere.

I found a cool trick for referencing between source files so that for all the
private variables representing state, I was able to refer to their counterpart
in the unnamed namespace of IceClFlags.cpp.

[rkotlerimgtec, 2015-12-16 04:28:56]

On 2015/12/16 04:25:56, rkotlerimgtec wrote:
> Here is a cut at making the IceClFlags.h very readable in the documentation
and
> where the IceClFlags class members are used elsewhere.
> 
> I found a cool trick for referencing between source files so that for all the
> private variables representing state, I was able to refer to their counterpart
> in the unnamed namespace of IceClFlags.cpp.

This option I use for referencing another file is fairly new so you may need a
current version of Doxygen to see it working.

I have 1.8.10

It seems to also be working in 1.8.6

[Jim Stichnoth, 2015-12-17 20:31:07]

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode11
src/IceClFlags.h:11: /// \brief Declares class Ice::ClFlags .
This comment doesn't seem particularly useful, on the order of something like:

  i++; // increment i

The previous comment seemed more useful.

But I'm not familiar enough with common doxygen usage, maybe this is how it's
normally done?

I guess there is information that the class declares *only* ClFlags.

Also, the original phrase "controlling translation" is basically moved down to
the class definition, so that's good.

It would be nice to retain the "command line" phrase, since the variables are
set on the command line.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode31
src/IceClFlags.h:31: /// User defined constructor.
What does "user defined" mean in this context?

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode34
src/IceClFlags.h:34: /// \brief Parse commmand line options for Subzero
I would be happier if all these \brief descriptions ended with a period.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode36
src/IceClFlags.h:36: /// This is done use cl::ParseCommandLineOptions and the
static variables of type
s/use/using/ ?

Maybe say cl::ParseCommandLineOptions() to make it clear that it is a function
call?

Also, 80-col.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode37
src/IceClFlags.h:37: /// cl::opt defined in IceClFlags.cpp
Is there a way to markup IceClFlags.cpp so that a link to that file's
documentation is automatically inserted?

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:390: /// Always false.
I don't think so:

unittest/BitcodeMunge.cpp:  Flags.setGenerateUnitTestMessages(true);

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:424: /// see
anonymous_namespace{IceClFlags.cpp}::RandomizeAndPoolImmediatesThreshold
80-col

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:426: /// Always 0
I think pnacl-sz -max-nops-per-instruction=123 changes this.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:428: /// Always 0
Same as above, I think there's another command-line option to control it.

[rkotlerimgtec, 2015-12-17 22:03:48]

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode11
src/IceClFlags.h:11: /// \brief Declares class Ice::ClFlags .
On 2015/12/17 20:31:07, stichnot wrote:
> This comment doesn't seem particularly useful, on the order of something like:
> 
>   i++; // increment i
> 
> The previous comment seemed more useful.
> 
> But I'm not familiar enough with common doxygen usage, maybe this is how it's
> normally done?
> 
> I guess there is information that the class declares *only* ClFlags.
> 
> Also, the original phrase "controlling translation" is basically moved down to
> the class definition, so that's good.
> 
> It would be nice to retain the "command line" phrase, since the variables are
> set on the command line. 

The idea is that file may contain the definition of many classes and so this is
just explaining what the file is about. Usually people are looking at the class
so that is where the more detailed information about what the class does would
be.

I'm not fixed on this way I did it, but that is my reasoning.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode31
src/IceClFlags.h:31: /// User defined constructor.
On 2015/12/17 20:31:07, stichnot wrote:
> What does "user defined" mean in this context?

It means it's not the default one the language supplies.
That is the c++ technical term so if someone sees a reference
to the constructor in other parts of the code, they will know it's not
the predefined one, since that one would have the same signature.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode34
src/IceClFlags.h:34: /// \brief Parse commmand line options for Subzero
On 2015/12/17 20:31:07, stichnot wrote:
> I would be happier if all these \brief descriptions ended with a period.

Done.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode36
src/IceClFlags.h:36: /// This is done use cl::ParseCommandLineOptions and the
static variables of type
On 2015/12/17 20:31:07, stichnot wrote:
> s/use/using/ ?
> 
> Maybe say cl::ParseCommandLineOptions() to make it clear that it is a function
> call?
> 
> Also, 80-col.

Done.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcode37
src/IceClFlags.h:37: /// cl::opt defined in IceClFlags.cpp
On 2015/12/17 20:31:07, stichnot wrote:
> Is there a way to markup IceClFlags.cpp so that a link to that file's
> documentation is automatically inserted?

I tried another variant here. Doxygen has lots of features.
There could well be something more clear that I could do here.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:390: /// Always false.
On 2015/12/17 20:31:07, stichnot wrote:
> I don't think so:
> 
> unittest/BitcodeMunge.cpp:  Flags.setGenerateUnitTestMessages(true);

Done.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:424: /// see
anonymous_namespace{IceClFlags.cpp}::RandomizeAndPoolImmediatesThreshold
On 2015/12/17 20:31:07, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:426: /// Always 0
On 2015/12/17 20:31:07, stichnot wrote:
> I think pnacl-sz -max-nops-per-instruction=123 changes this.

Done.

https://codereview.chromium.org/1527173002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:428: /// Always 0
On 2015/12/17 20:31:07, stichnot wrote:
> Same as above, I think there's another command-line option to control it.

Done.

[Jim Stichnoth, 2015-12-18 00:30:11]

lgtm

[Jim Stichnoth, 2015-12-18 00:30:32]

Description was changed from

==========
cleanup doxygen for IceClFlags.h

BUG=
==========

to

==========
cleanup doxygen for IceClFlags.h

BUG=
R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Jim Stichnoth, 2015-12-18 00:30:34]

Committed patchset #3 (id:40001) manually as
cb0d47b67fea89acba4dc14dc9ce19ee3f12132b (presubmit successful).