diff --git a/CMakeLists.txt b/CMakeLists.txt index 72bf5a95..3e50f0b5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -305,6 +305,10 @@ if(GENERATE_AUTO_JOB) add_custom_target(auto_job_files ALL DEPENDS ${auto_job_outputs}) endif() +add_test( + NAME check-assert + COMMAND perl ${qpdf_SOURCE_DIR}/check_assert) + # add_subdirectory order affects test order add_subdirectory(include) add_subdirectory(libqpdf) diff --git a/README-maintainer b/README-maintainer index 2e11a11b..88e81461 100644 --- a/README-maintainer +++ b/README-maintainer @@ -123,11 +123,23 @@ CODING RULES "Code Formatting" section in manual/contributing.rst for details. See also "CODE FORMATTING" below. -* Do not use assert in non-test code for any purpose other than as a - sanity check during development that would be safe to remove in - production. assert is for strong invariant checking. When developing - and using assert for that purpose, make sure to use the Debug - configuration since assert is disabled in other configurations. +* Use of assert: + + * Test code: #include first. + * Debug code: #include first and use + qpdf_assert_debug instead of assert. + + These rules are enforced by the check-assert test. This practices + serves to + + * remind us that assert in release code disappears and so should only + be used for debugging; when doing so use a Debug build + configuration + + * protect us from using assert in test code without explicitly + removing the NDEBUG definition, since that would cause the assert + not to actually be testing anything in non-Debug build + configurations. * In a source file, include the header file that declares the source class first followed by a blank line. If a config file is needed diff --git a/check_assert b/check_assert new file mode 100755 index 00000000..81ecf788 --- /dev/null +++ b/check_assert @@ -0,0 +1,62 @@ +#!/usr/bin/env perl +require 5.008; +use warnings; +use strict; +use File::Basename; + +my $whoami = basename($0); +chdir(dirname($0)) or die; + +my $errors = 0; +foreach my $file (glob('*/*.c'), glob('*/*.cc'), + glob('*/*/*.h'), glob('*/*/*.hh')) +{ + my $assert_test = 0; + if ($file =~ m,^libqpdf/qpdf/assert_,) + { + next; + } + open(F, "<$file") or die; + my $first_include = undef; + while () + { + if (m,^\s*#\s*include ,) + { + if ($1 eq 'test') + { + $assert_test = 1; + } + if (defined $first_include) + { + error("$file:$.: qpdf/assert header must be first"); + } + } + if (m,^\s*#\s*include <(.*?)>,) + { + my $header = $1; + if (($header eq 'cassert') || ($header eq 'assert.h')) + { + error("$file:$.: assert.h and cassert are not allowed --" . + " use one of the qpdf/assert_ files instead"); + } + $first_include = 1; + } + if ((! $assert_test) && (m/assert\(/)) + { + error("$file:$.: call qpdf_assert_debug instead of assert"); + } + } + close(F); +} +if ($errors) +{ + die "$whoami: errors detected\n"; +} +print "$whoami: no incorrect use of assert found\n"; + +sub error +{ + my $msg = shift; + warn $msg, "\n"; + $errors = 1; +} diff --git a/include/qpdf/QIntC.hh b/include/qpdf/QIntC.hh index ce2f47e9..dadc7582 100644 --- a/include/qpdf/QIntC.hh +++ b/include/qpdf/QIntC.hh @@ -24,7 +24,6 @@ #include #include -#include #include #include #include diff --git a/libqpdf/QPDFWriter.cc b/libqpdf/QPDFWriter.cc index e5270449..103abee1 100644 --- a/libqpdf/QPDFWriter.cc +++ b/libqpdf/QPDFWriter.cc @@ -1,4 +1,6 @@ -#include // include first for large file support +#include + +#include // include early for large file support #include @@ -21,7 +23,6 @@ #include #include -#include #include QPDFWriter::Members::Members(QPDF& pdf) : @@ -996,7 +997,7 @@ QPDFWriter::writePad(int nspaces) Pipeline* QPDFWriter::pushPipeline(Pipeline* p) { - assert(dynamic_cast(p) == 0); + qpdf_assert_debug(dynamic_cast(p) == 0); this->m->pipeline_stack.push_back(p); return p; } @@ -1027,9 +1028,9 @@ QPDFWriter::PipelinePopper::~PipelinePopper() if (stack_id.empty()) { return; } - assert(qw->m->pipeline_stack.size() >= 2); + qpdf_assert_debug(qw->m->pipeline_stack.size() >= 2); qw->m->pipeline->finish(); - assert( + qpdf_assert_debug( dynamic_cast(qw->m->pipeline_stack.back()) == qw->m->pipeline); // It might be possible for this assertion to fail if @@ -1038,7 +1039,7 @@ QPDFWriter::PipelinePopper::~PipelinePopper() // which two dynamically allocated PipelinePopper objects ever // exist at the same time, so the assertion will fail if they get // popped out of order from automatic destruction. - assert(qw->m->pipeline->getIdentifier() == stack_id); + qpdf_assert_debug(qw->m->pipeline->getIdentifier() == stack_id); delete qw->m->pipeline_stack.back(); qw->m->pipeline_stack.pop_back(); while (dynamic_cast(qw->m->pipeline_stack.back()) == 0) { @@ -1109,9 +1110,9 @@ QPDFWriter::pushMD5Pipeline(PipelinePopper& pp) throw std::logic_error("Deterministic ID computation enabled after ID" " generation has already occurred."); } - assert(this->m->deterministic_id); - assert(this->m->md5_pipeline == 0); - assert(this->m->pipeline->getCount() == 0); + qpdf_assert_debug(this->m->deterministic_id); + qpdf_assert_debug(this->m->md5_pipeline == 0); + qpdf_assert_debug(this->m->pipeline->getCount() == 0); this->m->md5_pipeline = new Pl_MD5("qpdf md5", this->m->pipeline); this->m->md5_pipeline->persistAcrossFinish(true); // Special case code in popPipelineStack clears this->m->md5_pipeline @@ -1123,8 +1124,8 @@ QPDFWriter::pushMD5Pipeline(PipelinePopper& pp) void QPDFWriter::computeDeterministicIDData() { - assert(this->m->md5_pipeline != 0); - assert(this->m->deterministic_id_data.empty()); + qpdf_assert_debug(this->m->md5_pipeline != 0); + qpdf_assert_debug(this->m->deterministic_id_data.empty()); this->m->deterministic_id_data = this->m->md5_pipeline->getHexDigest(); this->m->md5_pipeline->enable(false); } @@ -1786,7 +1787,7 @@ QPDFWriter::writeObjectStream(QPDFObjectHandle object) // object stream that we are generating from scratch. QPDFObjGen old_og = object.getObjGen(); - assert(old_og.getGen() == 0); + qpdf_assert_debug(old_og.getGen() == 0); int old_id = old_og.getObj(); int new_id = this->m->obj_renumber[old_og]; @@ -3003,7 +3004,7 @@ QPDFWriter::writeLinearized() closeObject(lindict_id); static int const pad = 200; int spaces = QIntC::to_int(pos - this->m->pipeline->getCount() + pad); - assert(spaces >= 0); + qpdf_assert_debug(spaces >= 0); writePad(spaces); writeString("\n"); @@ -3177,7 +3178,7 @@ QPDFWriter::writeLinearized() need_xref_stream ? 0 : 1); computeDeterministicIDData(); pp_md5 = 0; - assert(this->m->md5_pipeline == 0); + qpdf_assert_debug(this->m->md5_pipeline == 0); } // Close first pass pipeline @@ -3377,6 +3378,6 @@ QPDFWriter::writeStandard() "QPDFWriter standard deterministic ID", this->m->object_stream_to_objects.empty() ? 0 : 1); pp_md5 = 0; - assert(this->m->md5_pipeline == 0); + qpdf_assert_debug(this->m->md5_pipeline == 0); } } diff --git a/libqpdf/QPDF_encryption.cc b/libqpdf/QPDF_encryption.cc index 14b22d88..6a9ad871 100644 --- a/libqpdf/QPDF_encryption.cc +++ b/libqpdf/QPDF_encryption.cc @@ -1,6 +1,8 @@ // This file implements methods from the QPDF class that involve // encryption. +#include + #include #include @@ -15,7 +17,6 @@ #include #include -#include #include static unsigned char const padding_string[] = { @@ -288,7 +289,7 @@ hash_V5( ++round_number; std::string K1 = password + K + udata; - assert(K.length() >= 32); + qpdf_assert_debug(K.length() >= 32); std::string E = process_with_aes( K.substr(0, 16), true, diff --git a/libqpdf/QPDF_optimization.cc b/libqpdf/QPDF_optimization.cc index ac1bbfe6..4204a20d 100644 --- a/libqpdf/QPDF_optimization.cc +++ b/libqpdf/QPDF_optimization.cc @@ -1,12 +1,13 @@ // See the "Optimization" section of the manual. +#include + #include #include #include #include #include -#include QPDF::ObjUser::ObjUser() : ou_type(ou_bad), @@ -18,14 +19,14 @@ QPDF::ObjUser::ObjUser(user_e type) : ou_type(type), pageno(0) { - assert(type == ou_root); + qpdf_assert_debug(type == ou_root); } QPDF::ObjUser::ObjUser(user_e type, int pageno) : ou_type(type), pageno(pageno) { - assert((type == ou_page) || (type == ou_thumb)); + qpdf_assert_debug((type == ou_page) || (type == ou_thumb)); } QPDF::ObjUser::ObjUser(user_e type, std::string const& key) : @@ -33,7 +34,7 @@ QPDF::ObjUser::ObjUser(user_e type, std::string const& key) : pageno(0), key(key) { - assert((type == ou_trailer_key) || (type == ou_root_key)); + qpdf_assert_debug((type == ou_trailer_key) || (type == ou_root_key)); } bool diff --git a/libqpdf/qpdf/assert_debug.h b/libqpdf/qpdf/assert_debug.h new file mode 100644 index 00000000..0543d8fb --- /dev/null +++ b/libqpdf/qpdf/assert_debug.h @@ -0,0 +1,18 @@ +/* + * Include this file to use assert in regular code for + * debugging/strong sanity checking, knowing that the assert will be + * disabled in release code. Use qpdf_assert_debug in the code. + */ + +/* assert_debug and assert_test intentionally use the same + * guard. Search for assert in README-MAINTAINER. + */ +#ifdef QPDF_ASSERT_H +# error "At most one qpdf/assert header may be included at most one time" +#else +# define QPDF_ASSERT_H + +# include +# define qpdf_assert_debug assert + +#endif /* QPDF_ASSERT_H */ diff --git a/libqpdf/qpdf/assert_test.h b/libqpdf/qpdf/assert_test.h new file mode 100644 index 00000000..5310cdaa --- /dev/null +++ b/libqpdf/qpdf/assert_test.h @@ -0,0 +1,20 @@ +/* + * Include this file to use assert in regular code for + * debugging/strong sanity checking, knowing that the assert will be + * disabled in release code. Use qpdf_debug_assert in the code. + */ + +/* assert_debug and assert_test intentionally use the same + * guard. Search for assert in README-MAINTAINER. + */ +#ifdef QPDF_ASSERT_H +# error "At most one qpdf/assert header may be included at most one time" +#else +# define QPDF_ASSERT_H + +# ifdef NDEBUG +# undef NDEBUG +# endif +# include + +#endif /* QPDF_ASSERT_H */ diff --git a/libtests/arg_parser.cc b/libtests/arg_parser.cc index 8b4d556a..1480a717 100644 --- a/libtests/arg_parser.cc +++ b/libtests/arg_parser.cc @@ -1,15 +1,11 @@ +#include + #include #include #include #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - class ArgParser { public: diff --git a/libtests/base64.cc b/libtests/base64.cc index 66f2d828..0ce6f9a3 100644 --- a/libtests/base64.cc +++ b/libtests/base64.cc @@ -1,8 +1,9 @@ +#include + #include #include #include -#include #include #include #include diff --git a/libtests/buffer.cc b/libtests/buffer.cc index a24c0f99..a65efb1c 100644 --- a/libtests/buffer.cc +++ b/libtests/buffer.cc @@ -1,3 +1,5 @@ +#include + #include #include #include @@ -7,12 +9,6 @@ #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - static unsigned char* uc(char const* s) { diff --git a/libtests/concatenate.cc b/libtests/concatenate.cc index 10902adf..9bc0fd04 100644 --- a/libtests/concatenate.cc +++ b/libtests/concatenate.cc @@ -1,15 +1,11 @@ +#include + #include #include #include #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - static void pipeStringAndFinish(Pipeline* p, std::string const& str) { diff --git a/libtests/cxx11.cc b/libtests/cxx11.cc index 4c2c2cb6..250e7e2b 100644 --- a/libtests/cxx11.cc +++ b/libtests/cxx11.cc @@ -1,3 +1,5 @@ +#include + #include #include #include @@ -9,12 +11,6 @@ #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - // Functional programming // Function that returns a callable in the form of a lambda diff --git a/libtests/json.cc b/libtests/json.cc index 43f2b3c6..3844222f 100644 --- a/libtests/json.cc +++ b/libtests/json.cc @@ -1,13 +1,9 @@ +#include + #include #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - static void check(JSON const& j, std::string const& exp) { diff --git a/libtests/json_handler.cc b/libtests/json_handler.cc index 567cde86..4e610b08 100644 --- a/libtests/json_handler.cc +++ b/libtests/json_handler.cc @@ -1,14 +1,10 @@ +#include + #include #include #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - static void print_null(std::string const& path) { diff --git a/libtests/matrix.cc b/libtests/matrix.cc index 7dc73390..f69d270c 100644 --- a/libtests/matrix.cc +++ b/libtests/matrix.cc @@ -1,13 +1,9 @@ +#include + #include #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - static void check(QPDFMatrix const& m, std::string const& exp) { diff --git a/libtests/pdf_version.cc b/libtests/pdf_version.cc index e9bfb00b..5db444b3 100644 --- a/libtests/pdf_version.cc +++ b/libtests/pdf_version.cc @@ -1,13 +1,9 @@ +#include + #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - int main() { diff --git a/libtests/predictors.cc b/libtests/predictors.cc index c024cbd6..a8f6c5d9 100644 --- a/libtests/predictors.cc +++ b/libtests/predictors.cc @@ -1,3 +1,5 @@ +#include + #include #include #include @@ -9,12 +11,6 @@ #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - void run(char const* filename, char const* filter, diff --git a/libtests/qintc.cc b/libtests/qintc.cc index d3e19f5b..d5738a4b 100644 --- a/libtests/qintc.cc +++ b/libtests/qintc.cc @@ -1,12 +1,8 @@ +#include + #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - #define try_convert(exp_pass, fn, i) \ try_convert_real(#fn "(" #i ")", exp_pass, fn, i) diff --git a/libtests/qutil.cc b/libtests/qutil.cc index 017f371b..f79f9a3f 100644 --- a/libtests/qutil.cc +++ b/libtests/qutil.cc @@ -1,3 +1,5 @@ +#include + #include #include #include @@ -17,12 +19,6 @@ # include #endif -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - template void test_to_number( diff --git a/libtests/rc4.cc b/libtests/rc4.cc index 7245ef86..1b1a8744 100644 --- a/libtests/rc4.cc +++ b/libtests/rc4.cc @@ -1,3 +1,5 @@ +#include + #include #include #include @@ -8,12 +10,6 @@ #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - static void other_tests() { diff --git a/libtests/sparse_array.cc b/libtests/sparse_array.cc index 8d37bc2f..62410399 100644 --- a/libtests/sparse_array.cc +++ b/libtests/sparse_array.cc @@ -1,12 +1,8 @@ +#include + #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - int main() { diff --git a/qpdf/CMakeLists.txt b/qpdf/CMakeLists.txt index 94cf9e2d..bc4ddf11 100644 --- a/qpdf/CMakeLists.txt +++ b/qpdf/CMakeLists.txt @@ -29,6 +29,16 @@ endforeach() target_include_directories(qpdf-ctest PRIVATE ${CMAKE_BINARY_DIR}/libqpdf) target_include_directories(sizes PRIVATE ${JPEG_INCLUDE}) +set(needs_private_headers + test_large_file + test_driver + qpdf-ctest + qpdfjob-ctest + ) +foreach(TARGET ${needs_private_headers}) + target_include_directories(${TARGET} PRIVATE ${CMAKE_SOURCE_DIR}/libqpdf) +endforeach() + foreach(B qpdf test_unicode_filenames fix-qdf test_shell_glob) if(WINDOWS_WMAIN_COMPILE) target_compile_options(${B} PRIVATE ${WINDOWS_WMAIN_COMPILE}) diff --git a/qpdf/qpdf-ctest.c b/qpdf/qpdf-ctest.c index 62274c52..07d6ca9e 100644 --- a/qpdf/qpdf-ctest.c +++ b/qpdf/qpdf-ctest.c @@ -1,3 +1,5 @@ +#include + #include #include #include @@ -6,12 +8,6 @@ #include // for LL_FMT -- special case in build -#ifdef NDEBUG -/* We need assert even in a release build for test code. */ -# undef NDEBUG -#endif -#include - static char* whoami = 0; static qpdf_data qpdf = 0; diff --git a/qpdf/qpdfjob-ctest.c b/qpdf/qpdfjob-ctest.c index e67890e8..8d5e374e 100644 --- a/qpdf/qpdfjob-ctest.c +++ b/qpdf/qpdfjob-ctest.c @@ -1,14 +1,10 @@ +#include + #include #include #include #include -#ifdef NDEBUG -/* We need assert even in a release build for test code. */ -# undef NDEBUG -#endif -#include - #ifndef QPDF_NO_WCHAR_T static void wide_test() diff --git a/qpdf/test_driver.cc b/qpdf/test_driver.cc index f0c56176..1934b02b 100644 --- a/qpdf/test_driver.cc +++ b/qpdf/test_driver.cc @@ -1,3 +1,5 @@ +#include + // This program tests miscellaneous functionality in the qpdf library // that we don't want to pollute the qpdf program with. @@ -32,12 +34,6 @@ #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - static char const* whoami = 0; void diff --git a/qpdf/test_large_file.cc b/qpdf/test_large_file.cc index 32494426..904f3dd6 100644 --- a/qpdf/test_large_file.cc +++ b/qpdf/test_large_file.cc @@ -1,3 +1,5 @@ +#include + // NOTE: This test program doesn't do anything special to enable large // file support. This is important since it verifies that programs // don't have to do anything special -- all the work is done @@ -14,12 +16,6 @@ #include #include -#ifdef NDEBUG -// We need assert even in a release build for test code. -# undef NDEBUG -#endif -#include - // Run "test_large_file write small a.pdf" to get a PDF file that you // can look at in a reader.