-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Correct documentation mistakes in double.h and long_double.h #8729
base: master
Are you sure you want to change the base?
Correct documentation mistakes in double.h and long_double.h #8729
Conversation
It should even be FieldWithKthRoot, and not just FieldWithSqrt. |
Note a possible related issue is #7782 |
I did the modification, now the PR also solved issue 7782 |
I don't yet see a change for |
@@ -11,4 +11,5 @@ This header provides all necessary functions so the fundamental types | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the above lines still necessary or should they be reformulated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that documentation page (see https://doc.cgal.org/latest/Number_types/int_8h.html) is not only for int
, but also for short int
and long int
. It's not the documentation of a specific type like the others but of a header. Looking at the complete doc, I would have in fact gone the opposite way and removed all the cgalModels
from these header documentations...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the suggestion of Mael, now there is only one sentence instead of four identicals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be something like:
\cgalModels{RealEmbeddable,EuclideanRing}
as well, so it is consistent with the other files?
I think the \cgalModels{RealEmbeddable,EuclideanRing}
is good to have as you see it at once and don't have to look through the text (although this is quite small in this case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I aggry I understand the suggestion of mael as removing the three sentences "int is model of ..., short int is model of ..., ..., long int is model of ...".
I think it is good not to waste too much energy on these very small files. If you think that the actual change does not miss anything important, we can close the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good not to waste too much energy on these very small files.
I understand what you are saying, but consistency is a great thing, quite often the first impression that a user gets is through the documentation. Also consistency can make things easier in the future. So it looks like wasting time but I think it pays off in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\cgalModels{RealEmbeddable,EuclideanRing}
displays:
Is model of
That is appropriate for a documentation page about one single entity. But here CGAL/int.h
is about multiple integral types and \cgalModels
cannot be used, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it just the word "Is" that should be "Are" or is it more fundamental?
- In the first case a second
ALIASES
would do. - In the fundamental case I don't see a quick solution (I don't need the fundamental explanation).
Release Management