Skip to content
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

DOC: Add "Integer Type Specifiers" section #183

Merged

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Apr 8, 2022

Added coding style guidelines for specifying built-in integer types and
integer types from the C library.

Following recent ITK commits, authored by Lee Newberg (@Leengit):

pull request InsightSoftwareConsortium/ITK#3365
commit InsightSoftwareConsortium/ITK@8c2c654
commit InsightSoftwareConsortium/ITK@e03a941
"STYLE: Drop signed and int from some types"

pull request InsightSoftwareConsortium/ITK#3139
commit InsightSoftwareConsortium/ITK@a812aa3
"STYLE: Change unsigned to unsigned int."

pull request InsightSoftwareConsortium/ITK#3250
commit InsightSoftwareConsortium/ITK@c173dfd
"STYLE: Remove std:: prefix from types that work without it"

@github-actions github-actions bot added area:Appendices Issues affecting the Appendices part area:Documentation Issues affecting the Documentation module language:LaTeX Changes to LaTeX code type:BookStyle Changes to book style files labels Apr 8, 2022
@Leengit
Copy link
Contributor

Leengit commented Apr 8, 2022

Excellent. Is this the right section to also state that when each is needed static or extern should be first; then const, constexpr, or mutable; then unsigned (but only in the case of char, short, int, long, or long long) or signed (but only in the case of char); and finally bool, char, short, int, long, long long, float, double, or long double.

dzenanz
dzenanz previously approved these changes Apr 8, 2022
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this.

The quotes should be formatted in agreement with LaTeX syntax, or else the inline verbatim/code syntax should be used.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Apr 8, 2022

Excellent. Is this the right section to also state that when each is needed static or extern should be first; then const, constexpr, or mutable; then unsigned (but only in the case of char, short, int, long, or long long) or signed (but only in the case of char); and finally bool, char, short, int, long, long long, float, double, or long double.

Thanks for the suggestion, but I think this should be in a different section. Because the section I'm proposing here is specifically about integer types.

@Leengit
Copy link
Contributor

Leengit commented Apr 8, 2022

... should be in a different section.

For when we do include that information, there's a page that suggests the order for even more categories: attributes-friendness-storage-constness-virtualness-explicitness-signedness-length-type.

@N-Dekker
Copy link
Contributor Author

The quotes should be formatted in agreement with LaTeX syntax, or else the inline verbatim/code syntax should be used.

@jhlegarreta Thanks for you comment, Jon. Would those single "backtick" quotes that I'm using here (``) cause LaTeX syntax errors, or it is only a matter of style? Many other applications support those backticks for code, including GitHub/MarkDown: https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#quoting-code

@Leengit
Copy link
Contributor

Leengit commented Apr 11, 2022

Just in case it isn't clear from the previous posts here ... my summary:

LaTeX wants it to be a pair of single back ticks and then a pair of single forward ticks:

``unsigned int''

if the goal is to put it in double quotes. If instead the goal is to use a fixed-width font then the likes of

\code{unsigned int}

is defined for this document.

@jhlegarreta
Copy link
Member

@jhlegarreta Thanks for you comment, Jon. Would those single "backtick" quotes that I'm using here (``) cause LaTeX syntax errors, or it is only a matter of style? Many other applications support those backticks for code, including GitHub/MarkDown: https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#quoting-code

@N-Dekker Lee's comment explains it: we are not writing Markdown text here, but LaTeX text. The single backticks in LaTeX do not translate into any markup/highlighting, and thus, if the text within single backticks is intended to be highlighted somehow (as it is the case), using single backticks has no effect.

dzenanz
dzenanz previously approved these changes Apr 11, 2022
@N-Dekker
Copy link
Contributor Author

@Leengit

For when we do include that information, there's a page that suggests the order for even more categories: https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife

Very interesting, thanks Lee, but again, I feel it's beyond the scope of this particular pull request. If you feel like preparing another PR specifically on keyword ordering, feel free to do so 😃

@N-Dekker
Copy link
Contributor Author

build-publish-pdf is still failing, after having fixed the quotes and double-quotes (hereby force-pushed), as requested by @jhlegarreta and @Leengit It says:

ninja: build stopped: subcommand failed

Does anyone have a clue?

@dzenanz
Copy link
Member

dzenanz commented Apr 11, 2022

ITKDevelopmentEnvironment seems to be building the nightly one. The last one errored out, #184 is an attempt to address that.

@dzenanz
Copy link
Member

dzenanz commented Apr 11, 2022

I took a look at the build log. Here is an abbreviated version:

[23/25] Performing build step for 'ITKSoftwareGuide'
FAILED: ITKSoftwareGuide-prefix/src/ITKSoftwareGuide-stamp/ITKSoftwareGuide-build /home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/build/ITKSoftwareGuide-prefix/src/ITKSoftwareGuide-stamp/ITKSoftwareGuide-build 
cd /home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/build/ITKSoftwareGuide-build && /usr/local/bin/cmake --build . && /usr/local/bin/cmake -E touch /home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/build/ITKSoftwareGuide-prefix/src/ITKSoftwareGuide-stamp/ITKSoftwareGuide-build
[1/114] Generating NeighborhoodConnectedImageFilterOutput1.eps

...

[101/114] Generating ITKSoftwareGuide-Book1.dvi, ITKSoftwareGuide-Book1.aux
FAILED: SoftwareGuide/Latex/ITKSoftwareGuide-Book1.dvi SoftwareGuide/Latex/ITKSoftwareGuide-Book1.aux /home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/build/ITKSoftwareGuide-build/SoftwareGuide/Latex/ITKSoftwareGuide-Book1.dvi /home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/build/ITKSoftwareGuide-build/SoftwareGuide/Latex/ITKSoftwareGuide-Book1.aux 
cd /home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/build/ITKSoftwareGuide-build/SoftwareGuide/Latex && /bin/sh /home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/build/ITKSoftwareGuide-build/SoftwareGuide/LaTeXWrapper.sh /home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/SoftwareGuide/Latex/ITKSoftwareGuide-Book1.tex
This is pdfTeX, Version 3.14159265-2.6-1.40.18 (TeX Live 2017/Debian) (preloaded format=latex)
 \write18 enabled.
entering extended mode

(/home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/SoftwareGuide/Latex/ITKSof
twareGuide-Book1.tex
LaTeX2e <2017-04-15>
Babel <3.18> and hyphenation patterns for 3 language(s) loaded.

(/home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/SoftwareGuide/../Latex/Ins
ightSoftwareGuide.cls
Document Class: InsightSoftwareGuide 1998/03/03 Document class (Insight Softwar
eGuide)
(/usr/share/texlive/texmf-dist/tex/latex/psnfss/times.sty)
Using Times instead of Computer Modern.
(/usr/share/texlive/texmf-dist/tex/latex/base/book.cls
Document Class: book 2014/09/29 v1.4h Standard LaTeX document class
(/usr/share/texlive/texmf-dist/tex/latex/base/bk10.clo))
Dimension textheight = 550.0pt
Dimension paperheight = 794.96999pt
Dimension topmargin = 22.0pt
(/usr/share/texlive/texmf-dist/tex/latex/fancyhdr/fancyhdr.sty)
Using fancier footers than usual.
(/usr/share/texlive/texmf-dist/tex/latex/fncychap/fncychap.sty)
Using fancy chapter headings.

(/home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/SoftwareGuide/../Latex/Ins
ight.sty (/usr/share/texlive/texmf-dist/tex/latex/tools/longtable.sty)

...

(/home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/SoftwareGuide/Latex/Introd
uction/Introduction.tex
Chapter 1.

LaTeX Warning: Reference `sec:AdditionalResources' on page 3 undefined on input
 line 25.


LaTeX Warning: Reference `sec:AdditionalResources' on page 3 undefined on input
 line 25.


...

a bunch more LaTeX warnings

...

</home/runner/work/ITKSoftwareGuide/ITKSoftwareGuide/build/ITKSoftwareGuide-bui
ld/SoftwareGuide/Art/Generated/ScalarImageMarkovRandomField1Output.eps>

LaTeX Warning: Reference `fig:ScalarImageMarkovRandomFieldInputOutput' on page 
525 undefined on input line 316.

)) [525] [526]This is makeindex, version 2.15 [TeX Live 2017] (kpathsea + Thai support).
Scanning input file ITKSoftwareGuide-Book2.idx.....done (1119 entries accepted, 0 rejected).
Sorting entries............done (12175 comparisons).
Generating output file ITKSoftwareGuide-Book2.ind.....done (1186 lines written, 0 warnings).
Output written in ITKSoftwareGuide-Book2.ind.
Transcript written in ITKSoftwareGuide-Book2.ilg.

No file ITKSoftwareGuide-Book2.bbl.
(./ITKSoftwareGuide-Book2.ind [527] [528] [529] [530] [531] [532] [533]
[534] [535]
Overfull \hbox (1.82451pt too wide) in paragraph at lines 801--803
[]\OT1/ptm/m/n/10 itk::Statistics::GaussianMembershipFunction, 

Overfull \hbox (1.19455pt too wide) in paragraph at lines 840--841
[]\OT1/ptm/m/n/10 itk::Statistics::PointSetToListSampleAdaptor, 
[536] [537] [538] [539]
Overfull \hbox (19.31503pt too wide) in paragraph at lines 1116--1117
[]| \OT1/ptm/m/n/10 itk::LaplacianRecursiveGaussianImageFilter, 
)

Package caption Warning: Unused \captionsetup[lstlisting] on input line 88.
See the caption package documentation for explanation.

[540] (./ITKSoftwareGuide-Book2.aux)

Package rerunfilecheck Warning: File `ITKSoftwareGuide-Book2.out' has changed.
(rerunfilecheck)                Rerun to get outlines right
(rerunfilecheck)                or use package `bookmark'.


LaTeX Warning: There were undefined references.


LaTeX Warning: Label(s) may have changed. Rerun to get cross-references right.

 )
(see the transcript file for additional information)
Output written on ITKSoftwareGuide-Book2.dvi (554 pages, 3330704 bytes).
Transcript written on ITKSoftwareGuide-Book2.log.
ninja: build stopped: subcommand failed.
ninja: build stopped: subcommand failed.
Error: Process completed with exit code 1.

\label{subsec:IntegerTypeSpecifiers}

Throughout the code base, the built-in type \code{unsigned int} is spelled exactly like that:
``unsigned int'' (rather than just ``unsigned''). Other built-in integer types should in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort @N-Dekker. Why are some types left with quotes instead of consistently using the \code{} markup? To me, these are programming types/constructs not found in English language (as a whole) that deserve such highlighting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhlegarreta Thanks for asking. I did deliberately make a choice between \code{...} and double-quotes, each time. You see, this section specifically deals with how to spell a certain type.

  • \code{unsigned int} refers to the C++ type itself

  • "unsigned int" refers to the way we spell it (according to our Style Guide).

Both spellings, "unsigned" and "unsigned int" denote the very same C++ type, unsigned int.

Hope that answers your question. 😃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not convinced. If others think it is OK as it is, then it's maybe a very personal view of mine, and the conversation can be resolved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If others think it is OK ...

Whichever. I am willing to defer to the person doing the work.

@N-Dekker
Copy link
Contributor Author

In a way, this guideline treats "unsigned int" versus "unsigned" different than "signed long int" versus "long". Personally I would have preferred to consistently always use the shortest form (just "unsigned", like we do just "long"), but there appeared no consensus for just "unsigned", at PR InsightSoftwareConsortium/ITK#3139. So this guideline aims to describe the current ITK coding convention, not necessarily my personal preference 😺

I'm just saying so, because it's different from most of my other pull requests, which are usually mostly according to my preference 😄 No need to redo the "unsigned int" versus "unsigned" discussion right now.

@N-Dekker
Copy link
Contributor Author

FYI, My most recent amend (force-push) is just a git rebase master

@N-Dekker N-Dekker marked this pull request as ready for review April 13, 2022 14:36
@N-Dekker N-Dekker force-pushed the IntegerTypeSpecifiers branch from 0487224 to f0cc03c Compare November 11, 2022 09:02
@github-actions github-actions bot added the type:Documentation Documentation improvement or change label Nov 11, 2022
facilities (like \code{<cstdlib>} and \code{<cstdint>}) should generally be used without a \code{std::} prefix.
For example, ``size_t'', ``int8_t'', and ``uintmax_t'' are preferred to ``std::size_t'',
``std::int8_t'', and ``std::uintmax_t''. When using any of those type aliases, the ITK header file
``itkIntTypes.h'' may need to be \code{#include}d, to ensure that those types are declared within the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the # needs to be escaped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, the underscores needed to be escaped! For example, "size_t" needed to be spelled size\_t

@N-Dekker N-Dekker force-pushed the IntegerTypeSpecifiers branch from f0cc03c to a49725e Compare January 10, 2023 15:43
@N-Dekker N-Dekker closed this Feb 28, 2023
@N-Dekker N-Dekker deleted the IntegerTypeSpecifiers branch February 28, 2023 18:57
@N-Dekker N-Dekker restored the IntegerTypeSpecifiers branch February 28, 2023 19:09
@N-Dekker N-Dekker reopened this Feb 28, 2023
Added coding style guidelines for specifying built-in integer types and
integer types from the C library.

Following recent ITK commits, authored by Lee Newberg:

pull request InsightSoftwareConsortium/ITK#3365
commit InsightSoftwareConsortium/ITK@8c2c654
commit InsightSoftwareConsortium/ITK@e03a941
"STYLE: Drop `signed` and `int` from some types"

pull request InsightSoftwareConsortium/ITK#3139
commit InsightSoftwareConsortium/ITK@a812aa3
"STYLE: Change unsigned to unsigned int."

pull request InsightSoftwareConsortium/ITK#3250
commit InsightSoftwareConsortium/ITK@c173dfd
"STYLE: Remove `std::` prefix from types that work without it"
@N-Dekker N-Dekker force-pushed the IntegerTypeSpecifiers branch from a49725e to 612adf5 Compare February 28, 2023 19:14
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔢 thanks for the persistence on this @N-Dekker 🎇

@thewtex thewtex merged commit e8ae54e into InsightSoftwareConsortium:master Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Appendices Issues affecting the Appendices part area:Documentation Issues affecting the Documentation module language:LaTeX Changes to LaTeX code type:BookStyle Changes to book style files type:Documentation Documentation improvement or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants