-
Notifications
You must be signed in to change notification settings - Fork 219
configure: optional build documentation #212
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
Conversation
49e2c49
to
4ad4f87
Compare
lib/snprintf.c
Outdated
@@ -1097,6 +1097,9 @@ fmtflt(char *str, size_t *len, size_t size, LDOUBLE fvalue, int width, | |||
infnan = (flags & PRINT_F_UP) ? "INF" : "inf"; | |||
|
|||
if (infnan != NULL) { | |||
int i = MAX_CONVERT_LENGTH - 1; | |||
while (i >= 0) /* Initialize buffer, do not use memset (compatility) */ | |||
iconvert[i--] = '\0'; |
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.
Why not use memset?
Also, why not do this at the beginning at line 1084?
Should the same be done for fconvert, at least for consistency?
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.
Also (not a requirement, just a request), could you send the patch back to the original author as well?
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.
Function memset() is not used anywhere in libcompat. So I figured maybe someone had already come to the conclusion that memset() is not available at every C installation or other compatibility issue. A little backwards logic here, I know. Just being overly careful.
However, memset() is used in libcheck in three places. If you say that it is okay to use memset(), I'm all for it.
I will move it to the beginning of the function, line 1084. A good idea, since the buffer is also used later in the function.
Same for fconvert? I will change that too.
src/check_error.c
Outdated
* We need the GNU C version to prevent using pragmas where they are | ||
* not supported. | ||
*/ | ||
#define GCC_VERSION (__GNUC__ * 10000 \ |
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.
Should this be in libcompat instead? Or, is one goal to reduce/remove that header eventually?
lib/libcompat.h
Outdated
@@ -45,6 +45,11 @@ | |||
#define CK_ATTRIBUTE_NORETURN | |||
#endif /* GCC 2.5 */ | |||
|
|||
/* Actually GCC 4.6.4 */ |
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.
Oh, you did move it to the header.
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 mean to improve the GCC_VERSION_AT_LEAST(major, minor)
macro to GCC_VERSION_AT_LEAST(major, minor, patch)
because the patch number is useful in some circumstances, such as here.
I see that the Travis-CI build stopped working on the optional docs change. Does there need to be another example build to check that the docs can still be built, and that the docs can be skipped? |
Commit "configure: optional build documentation" does add a new feature, even if it is only in build. @brarcher , can you add it to Travis? |
The script which is run on Travis-CI is the travis.sh script. I'd suggest adding the following to the
path around line 39:
The rest of the test can proceed as normal. |
Fix issue libcheck#206 Applied an uncommitted patch by Branden Archer. https://sourceforge.net/p/check/bugs/101/ Signed-off-by: Mikko Johannes Koivunalho <mikko.koivunalho@iki.fi>
a73137d
to
486bddc
Compare
Add more output when running `./configure`. Signed-off-by: Mikko Johannes Koivunalho <mikko.koivunalho@iki.fi>
Tweak Travis build to support the new `./configure` switch. Signed-off-by: Mikko Johannes Koivunalho <mikko.koivunalho@iki.fi>
libcheck#206 Signed-off-by: Mikko Johannes Koivunalho <mikko.koivunalho@iki.fi>
486bddc
to
c4e5c3c
Compare
Missing the issue #217 . |
If you would like to postpone merging this until both CMake and Autotools both have a configurable option to build the docs, that is fine. I'll leave it to your discretion. Just let me know when it is ready to merge. |
The documentation itself needs to be revised. Part of the example needs to be rewritten so show how the new CMake import works. I have improved the file FindCheck.cmake to follow new CMake best practices. It is used to find Check installation from CMake build when the build does not support CMake style export/import or the using build wants to use an earlier (0.12.0 or earlier) Autotools built installation. |
AC_MSG_WARN(tex not installed: cannot rebuild HTML documentation.) | ||
|
||
if test "xtrue" = x"$enable_build_docs"; then | ||
AC_CHECK_PROGS(TEX, tex, false) |
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.
Why does this check for tex instead of makeinfo? The issue you referenced also mentions makeinfo, not tex. Your doc makefile also doesn't seem to call tex explicitly instead it uses the implicit GNU make rule which uses makeinfo.
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.
Thanks for pointing this out, and thanks for your fix in #231
Fix issue #206
Applied an uncommitted patch by Branden Archer.
https://sourceforge.net/p/check/bugs/101/
Signed-off-by: Mikko Johannes Koivunalho mikko.koivunalho@iki.fi