{"thread":{"id":"47304","subject":"[PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro","startedAt":"2017-11-22T22:38:38Z","lastAt":"2017-11-23T05:00:16Z","messageCount":18,"participants":["Stefan Beller","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"333339","messageId":"20171122223827.26773-3-sbeller@google.com","threadId":"47304","inReplyTo":"20171122223827.26773-1-sbeller@google.com","subject":"[PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-22T22:38:26Z","receivedAt":"2017-11-22T22:38:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A lot of BUG() invocations are in the form of\n    if (condition)\n        BUG()\nso moving the condition into the same line as the macro will result in\nmore readable code.  The conversion to use this MACRO will happen in a\nlater patch.\n\nThis macro in name and spirit is borrowed from linux, which defines it as\n\n    #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)\n\nin include/asm-generic/bug.h, however Git prefers to have a more specific\nmessage in BUG() calls that we include as well.\n\nIn case the user doesn't have HAVE_VARIADIC_MACROS, I could not come up\nwith some MACRO trickery to transport the message down to BUG conditionally\nsuch that BUG_ON is a function just like BUG() as well.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-compat-util.h |  4 ++++\n usage.c           | 12 +++++++++++-\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cedad4d581..4fec462e30 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n __attribute__((format (printf, 3, 4))) NORETURN\n void BUG_fl(const char *file, int line, const char *fmt, ...);\n #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)\n+#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while (0)\n #else\n __attribute__((format (printf, 1, 2))) NORETURN\n void BUG(const char *fmt, ...);\n+\n+__attribute__((format (printf, 2, 3)))\n+void BUG_ON(int condition, const char *fmt, ...);\n #endif\n \n /*\ndiff --git a/usage.c b/usage.c\nindex cdd534c9df..3aed669181 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -240,7 +240,17 @@ NORETURN void BUG(const char *fmt, ...)\n \tBUG_vfl(NULL, 0, fmt, ap);\n \tva_end(ap);\n }\n-#endif\n+\n+void BUG_ON(int condition, const char *fmt, ...)\n+{\n+\tif (condition) {\n+\t\tva_list ap;\n+\t\tva_start(ap, fmt);\n+\t\tBUG_vfl(NULL, 0, fmt, ap);\n+\t\tva_end(ap);\n+\t}\n+}\n+#endif /* HAVE_VARIADIC_MACROS */\n \n #ifdef SUPPRESS_ANNOTATED_LEAKS\n void unleak_memory(const void *ptr, size_t len)\n-- \n2.15.0.448.gf294e3d99a-goog\n\n"},{"id":"333340","messageId":"20171122223827.26773-1-sbeller@google.com","threadId":"47304","inReplyTo":null,"subject":"[PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-22T22:38:24Z","receivedAt":"2017-11-22T22:38:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On reviewing [1] I wondered why there are so many asserts and wondered\nif these asserts could have been prevented by a better functionality around\nbug reporting in our code.\n\nIntroduce a BUG_ON macro, which is superior to assert() by\n * being always there, even when compiled with NDEBUG and\n * providind an additional human readable error message, like BUG()\n \nOpinions?\n\nThanks,\nStefan\n\n[1] https://public-inbox.org/git/20171121205852.15731-5-git@jeffhostetler.com/\n\nStefan Beller (3):\n  Documentation/CodingGuidelines: explain why assert is bad\n  git-compat: introduce BUG_ON(condition, fmt, ...) macro\n  contrib/coccinelle: convert all conditional bugs to bug_on\n\n Documentation/CodingGuidelines  |  3 +++\n builtin/merge.c                 |  3 +--\n contrib/coccinelle/bug_on.cocci |  8 ++++++++\n environment.c                   | 22 ++++++++++------------\n git-compat-util.h               |  4 ++++\n notes.c                         |  9 +++++----\n refs.c                          |  7 +++----\n refs/files-backend.c            | 14 ++++++--------\n refs/packed-backend.c           | 13 +++++--------\n sha1_file.c                     |  4 ++--\n tempfile.c                      | 34 ++++++++++++++++------------------\n usage.c                         | 12 +++++++++++-\n 12 files changed, 74 insertions(+), 59 deletions(-)\n create mode 100644 contrib/coccinelle/bug_on.cocci\n\n-- \n2.15.0.448.gf294e3d99a-goog\n\n"},{"id":"333341","messageId":"20171122223827.26773-4-sbeller@google.com","threadId":"47304","inReplyTo":"20171122223827.26773-1-sbeller@google.com","subject":"[PATCH 3/3] contrib/coccinelle: convert all conditional bugs to bug_on","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-22T22:38:27Z","receivedAt":"2017-11-22T22:38:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Add a coccinelle patch that detects a conditional BUG and converts it to\nBUG_ON.  Also apply that semantic patch.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/merge.c                 |  3 +--\n contrib/coccinelle/bug_on.cocci |  8 ++++++++\n environment.c                   | 22 ++++++++++------------\n notes.c                         |  9 +++++----\n refs.c                          |  7 +++----\n refs/files-backend.c            | 14 ++++++--------\n refs/packed-backend.c           | 13 +++++--------\n sha1_file.c                     |  4 ++--\n tempfile.c                      | 34 ++++++++++++++++------------------\n 9 files changed, 56 insertions(+), 58 deletions(-)\n create mode 100644 contrib/coccinelle/bug_on.cocci\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 612dd7bfb6..df5884b4c1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -770,8 +770,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tstrbuf_addbuf(&msg, &merge_msg);\n \tstrbuf_addch(&msg, '\\n');\n-\tif (squash)\n-\t\tBUG(\"the control must not reach here under --squash\");\n+\tBUG_ON(squash, \"the control must not reach here under --squash\");\n \tif (0 < option_edit)\n \t\tstrbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);\n \tif (signoff)\ndiff --git a/contrib/coccinelle/bug_on.cocci b/contrib/coccinelle/bug_on.cocci\nnew file mode 100644\nindex 0000000000..e778d19e3c\n--- /dev/null\n+++ b/contrib/coccinelle/bug_on.cocci\n@@ -0,0 +1,8 @@\n+@@\n+expression E;\n+@@\n+- if (E)\n+-  BUG(\n++ BUG_ON(E,\n+   ...);\n+\ndiff --git a/environment.c b/environment.c\nindex 8fa032f307..9ba01a85ec 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -177,22 +177,20 @@ int have_git_dir(void)\n \n const char *get_git_dir(void)\n {\n-\tif (!the_repository->gitdir)\n-\t\tBUG(\"git environment hasn't been setup\");\n+\tBUG_ON(!the_repository->gitdir, \"git environment hasn't been setup\");\n \treturn the_repository->gitdir;\n }\n \n const char *get_git_common_dir(void)\n {\n-\tif (!the_repository->commondir)\n-\t\tBUG(\"git environment hasn't been setup\");\n+\tBUG_ON(!the_repository->commondir,\n+\t       \"git environment hasn't been setup\");\n \treturn the_repository->commondir;\n }\n \n const char *get_git_namespace(void)\n {\n-\tif (!namespace)\n-\t\tBUG(\"git environment hasn't been setup\");\n+\tBUG_ON(!namespace, \"git environment hasn't been setup\");\n \treturn namespace;\n }\n \n@@ -242,8 +240,8 @@ const char *get_git_work_tree(void)\n \n char *get_object_directory(void)\n {\n-\tif (!the_repository->objectdir)\n-\t\tBUG(\"git environment hasn't been setup\");\n+\tBUG_ON(!the_repository->objectdir,\n+\t       \"git environment hasn't been setup\");\n \treturn the_repository->objectdir;\n }\n \n@@ -282,15 +280,15 @@ int odb_pack_keep(const char *name)\n \n char *get_index_file(void)\n {\n-\tif (!the_repository->index_file)\n-\t\tBUG(\"git environment hasn't been setup\");\n+\tBUG_ON(!the_repository->index_file,\n+\t       \"git environment hasn't been setup\");\n \treturn the_repository->index_file;\n }\n \n char *get_graft_file(void)\n {\n-\tif (!the_repository->graft_file)\n-\t\tBUG(\"git environment hasn't been setup\");\n+\tBUG_ON(!the_repository->graft_file,\n+\t       \"git environment hasn't been setup\");\n \treturn the_repository->graft_file;\n }\n \ndiff --git a/notes.c b/notes.c\nindex c7f21fae44..cae8fa0657 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -400,10 +400,11 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t     oid_to_hex(&subtree->val_oid));\n \n \tprefix_len = subtree->key_oid.hash[KEY_INDEX];\n-\tif (prefix_len >= GIT_SHA1_RAWSZ)\n-\t\tBUG(\"prefix_len (%\"PRIuMAX\") is out of range\", (uintmax_t)prefix_len);\n-\tif (prefix_len * 2 < n)\n-\t\tBUG(\"prefix_len (%\"PRIuMAX\") is too small\", (uintmax_t)prefix_len);\n+\tBUG_ON(prefix_len >= GIT_SHA1_RAWSZ,\n+\t       \"prefix_len (%\"PRIuMAX\") is out of range\",\n+\t       (uintmax_t)prefix_len);\n+\tBUG_ON(prefix_len * 2 < n, \"prefix_len (%\"PRIuMAX\") is too small\",\n+\t       (uintmax_t)prefix_len);\n \tmemcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);\n \twhile (tree_entry(&desc, &entry)) {\n \t\tunsigned char type;\ndiff --git a/refs.c b/refs.c\nindex 339d4318ee..1a5473fbe3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -937,8 +937,8 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\tif (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n-\t\tBUG(\"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n+\tBUG_ON(flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS,\n+\t       \"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n \n \tflags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n \n@@ -1279,8 +1279,7 @@ struct ref_iterator *refs_ref_iterator_begin(\n \t\titer = prefix_ref_iterator_begin(iter, \"\", trim);\n \n \t/* Sanity check for subclasses: */\n-\tif (!iter->ordered)\n-\t\tBUG(\"reference iterator is not ordered\");\n+\tBUG_ON(!iter->ordered, \"reference iterator is not ordered\");\n \n \treturn iter;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f75d960e19..a00a7d9f78 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2208,8 +2208,8 @@ static int split_head_update(struct ref_update *update,\n \t * size, but it happens at most once per transaction.\n \t * Add new_update->refname instead of a literal \"HEAD\".\n \t */\n-\tif (strcmp(new_update->refname, \"HEAD\"))\n-\t\tBUG(\"%s unexpectedly not 'HEAD'\", new_update->refname);\n+\tBUG_ON(strcmp(new_update->refname, \"HEAD\"),\n+\t       \"%s unexpectedly not 'HEAD'\", new_update->refname);\n \titem = string_list_insert(affected_refnames, new_update->refname);\n \titem->util = new_update;\n \n@@ -2285,9 +2285,8 @@ static int split_symref_update(struct files_ref_store *refs,\n \t * referent, which might soon be freed by our caller.\n \t */\n \titem = string_list_insert(affected_refnames, new_update->refname);\n-\tif (item->util)\n-\t\tBUG(\"%s unexpectedly found in affected_refnames\",\n-\t\t    new_update->refname);\n+\tBUG_ON(item->util, \"%s unexpectedly found in affected_refnames\",\n+\t       new_update->refname);\n \titem->util = new_update;\n \n \treturn 0;\n@@ -2570,9 +2569,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tstruct string_list_item *item =\n \t\t\tstring_list_append(&affected_refnames, update->refname);\n \n-\t\tif ((update->flags & REF_IS_PRUNING) &&\n-\t\t    !(update->flags & REF_NO_DEREF))\n-\t\t\tBUG(\"REF_IS_PRUNING set without REF_NO_DEREF\");\n+\t\tBUG_ON((update->flags & REF_IS_PRUNING) && !(update->flags & REF_NO_DEREF),\n+\t\t       \"REF_IS_PRUNING set without REF_NO_DEREF\");\n \n \t\t/*\n \t\t * We store a pointer to update in item->util, but at\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 023243fd5f..e10c2f94d8 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -334,9 +334,7 @@ static void sort_snapshot(struct snapshot *snapshot)\n \n \twhile (pos < eof) {\n \t\teol = memchr(pos, '\\n', eof - pos);\n-\t\tif (!eol)\n-\t\t\t/* The safety check should prevent this. */\n-\t\t\tBUG(\"unterminated line found in packed-refs\");\n+\t\tBUG_ON(!eol, \"unterminated line found in packed-refs\");\n \t\tif (eol - pos < GIT_SHA1_HEXSZ + 2)\n \t\t\tdie_invalid_line(snapshot->refs->path,\n \t\t\t\t\t pos, eof - pos);\n@@ -349,9 +347,8 @@ static void sort_snapshot(struct snapshot *snapshot)\n \t\t\tconst char *peeled_start = eol;\n \n \t\t\teol = memchr(peeled_start, '\\n', eof - peeled_start);\n-\t\t\tif (!eol)\n-\t\t\t\t/* The safety check should prevent this. */\n-\t\t\t\tBUG(\"unterminated peeled line found in packed-refs\");\n+\t\t\tBUG_ON(!eol,\n+\t\t\t       \"unterminated peeled line found in packed-refs\");\n \t\t\teol++;\n \t\t}\n \n@@ -1272,8 +1269,8 @@ int is_packed_transaction_needed(struct ref_store *ref_store,\n \tsize_t i;\n \tint ret;\n \n-\tif (!is_lock_file_locked(&refs->lock))\n-\t\tBUG(\"is_packed_transaction_needed() called while unlocked\");\n+\tBUG_ON(!is_lock_file_locked(&refs->lock),\n+\t       \"is_packed_transaction_needed() called while unlocked\");\n \n \t/*\n \t * We're only going to bother returning false for the common,\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8a7c6b7eba..f10886f907 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1885,8 +1885,8 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \tint r = 0;\n \tstruct object_id oid;\n \n-\tif (subdir_nr > 0xff)\n-\t\tBUG(\"invalid loose object subdirectory: %x\", subdir_nr);\n+\tBUG_ON(subdir_nr > 0xff, \"invalid loose object subdirectory: %x\",\n+\t       subdir_nr);\n \n \toriglen = path->len;\n \tstrbuf_complete(path, '/');\ndiff --git a/tempfile.c b/tempfile.c\nindex 5fdafdd2d2..63884b6bdd 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -107,8 +107,8 @@ static void activate_tempfile(struct tempfile *tempfile)\n {\n \tstatic int initialized;\n \n-\tif (is_tempfile_active(tempfile))\n-\t\tBUG(\"activate_tempfile called for active object\");\n+\tBUG_ON(is_tempfile_active(tempfile),\n+\t       \"activate_tempfile called for active object\");\n \n \tif (!initialized) {\n \t\tsigchain_push_common(remove_tempfiles_on_signal);\n@@ -215,10 +215,9 @@ struct tempfile *xmks_tempfile_m(const char *template, int mode)\n \n FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)\n {\n-\tif (!is_tempfile_active(tempfile))\n-\t\tBUG(\"fdopen_tempfile() called for inactive object\");\n-\tif (tempfile->fp)\n-\t\tBUG(\"fdopen_tempfile() called for open object\");\n+\tBUG_ON(!is_tempfile_active(tempfile),\n+\t       \"fdopen_tempfile() called for inactive object\");\n+\tBUG_ON(tempfile->fp, \"fdopen_tempfile() called for open object\");\n \n \ttempfile->fp = fdopen(tempfile->fd, mode);\n \treturn tempfile->fp;\n@@ -226,22 +225,22 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)\n \n const char *get_tempfile_path(struct tempfile *tempfile)\n {\n-\tif (!is_tempfile_active(tempfile))\n-\t\tBUG(\"get_tempfile_path() called for inactive object\");\n+\tBUG_ON(!is_tempfile_active(tempfile),\n+\t       \"get_tempfile_path() called for inactive object\");\n \treturn tempfile->filename.buf;\n }\n \n int get_tempfile_fd(struct tempfile *tempfile)\n {\n-\tif (!is_tempfile_active(tempfile))\n-\t\tBUG(\"get_tempfile_fd() called for inactive object\");\n+\tBUG_ON(!is_tempfile_active(tempfile),\n+\t       \"get_tempfile_fd() called for inactive object\");\n \treturn tempfile->fd;\n }\n \n FILE *get_tempfile_fp(struct tempfile *tempfile)\n {\n-\tif (!is_tempfile_active(tempfile))\n-\t\tBUG(\"get_tempfile_fp() called for inactive object\");\n+\tBUG_ON(!is_tempfile_active(tempfile),\n+\t       \"get_tempfile_fp() called for inactive object\");\n \treturn tempfile->fp;\n }\n \n@@ -275,10 +274,9 @@ int close_tempfile_gently(struct tempfile *tempfile)\n \n int reopen_tempfile(struct tempfile *tempfile)\n {\n-\tif (!is_tempfile_active(tempfile))\n-\t\tBUG(\"reopen_tempfile called for an inactive object\");\n-\tif (0 <= tempfile->fd)\n-\t\tBUG(\"reopen_tempfile called for an open object\");\n+\tBUG_ON(!is_tempfile_active(tempfile),\n+\t       \"reopen_tempfile called for an inactive object\");\n+\tBUG_ON(0 <= tempfile->fd, \"reopen_tempfile called for an open object\");\n \ttempfile->fd = open(tempfile->filename.buf, O_WRONLY);\n \treturn tempfile->fd;\n }\n@@ -287,8 +285,8 @@ int rename_tempfile(struct tempfile **tempfile_p, const char *path)\n {\n \tstruct tempfile *tempfile = *tempfile_p;\n \n-\tif (!is_tempfile_active(tempfile))\n-\t\tBUG(\"rename_tempfile called for inactive object\");\n+\tBUG_ON(!is_tempfile_active(tempfile),\n+\t       \"rename_tempfile called for inactive object\");\n \n \tif (close_tempfile_gently(tempfile)) {\n \t\tdelete_tempfile(tempfile_p);\n-- \n2.15.0.448.gf294e3d99a-goog\n\n"},{"id":"333342","messageId":"20171122223827.26773-2-sbeller@google.com","threadId":"47304","inReplyTo":"20171122223827.26773-1-sbeller@google.com","subject":"[PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-22T22:38:25Z","receivedAt":"2017-11-22T22:38:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/CodingGuidelines | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex c4cb5ff0d4..4f8791895b 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -386,6 +386,9 @@ For C programs:\n  - Use Git's gettext wrappers to make the user interface\n    translatable. See \"Marking strings for translation\" in po/README.\n \n+ - Prefer the BUG() macro over asserts, as asserts requires that the\n+   NDEBUG flag is unset on compilation to work.\n+\n For Perl programs:\n \n  - Most of the C guidelines above apply.\n-- \n2.15.0.448.gf294e3d99a-goog\n\n"},{"id":"333344","messageId":"20171122225913.GF11671@aiede.mtv.corp.google.com","threadId":"47304","inReplyTo":"20171122223827.26773-2-sbeller@google.com","subject":"Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-22T22:59:13Z","receivedAt":"2017-11-22T22:59:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -386,6 +386,9 @@ For C programs:\n>   - Use Git's gettext wrappers to make the user interface\n>     translatable. See \"Marking strings for translation\" in po/README.\n>  \n> + - Prefer the BUG() macro over asserts, as asserts requires that the\n> +   NDEBUG flag is unset on compilation to work.\n\nnit: is there some logical place in the list of C guidelines this\nshould go at, instead of the last item?  Maybe near the top, since\nthis is one of those straightforward cases and we're just saying that\nthis is the startegy for asserting invariants that this project\nprefers.\n\nSeparately: I am not sure we currently universally prefer BUG_ON()\nover assert().  In theory, assert() is fine wherever you don't care\nwhether the assertion is checked at run-time --- in other words, it is\na fancy form of comment.  BUG_ON() is useful for defensive checks that\nyou *expect* to never trip but want to rely on afterwards.\n\nIn a certain ideal world, the preference would be reversed: you'd want\nto use assert() wherever you can and require the compiler to check\nthat all assert()s are verifiable at compile time.  A check that a\nstatic analyzer can verify is more valuable than a run-time check.\nWhen a compile-time check is not possible, you'd have to fall back to\nBUG_ON().\n\nBut we don't live in that ideal world.  I'm not aware of any widely\navailable tools for checking assert()s.  So maybe it makes sense to\nforbid assert() in our codebase entirely.\n\nThat could be enforced using something like check-non-portable-shell.pl,\nexcept\n\n- run on C files instead of sh files\n- run on Git's source code instead of t/\n\nWhat do you think?\n\nThanks,\nJonathan\n"},{"id":"333345","messageId":"20171122230239.GG11671@aiede.mtv.corp.google.com","threadId":"47304","inReplyTo":"20171122223827.26773-3-sbeller@google.com","subject":"Re: [PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-22T23:02:39Z","receivedAt":"2017-11-22T23:02:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n>  __attribute__((format (printf, 3, 4))) NORETURN\n>  void BUG_fl(const char *file, int line, const char *fmt, ...);\n>  #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)\n> +#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while (0)\n>  #else\n>  __attribute__((format (printf, 1, 2))) NORETURN\n>  void BUG(const char *fmt, ...);\n> +\n> +__attribute__((format (printf, 2, 3)))\n> +void BUG_ON(int condition, const char *fmt, ...);\n>  #endif\n\nI worry that these definitions are mildly incompatible: the macro\naccepts anything that can go in an 'if', including pointers, and the\nfunction only accepts an int.\n\nIs there a way for the macro to typecheck that its argument is an\ninteger to avoid that?\n\nThanks,\nJonathan\n"},{"id":"333346","messageId":"CAGZ79kY7yrgYSM7jLQD3Q4sRpjovXY=HZwU4M-Ai_XRPseda9A@mail.gmail.com","threadId":"47304","inReplyTo":"20171122225913.GF11671@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-22T23:08:00Z","receivedAt":"2017-11-22T23:08:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 22, 2017 at 2:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n> In a certain ideal world, the preference would be reversed: you'd want\n> to use assert() wherever you can and require the compiler to check\n> that all assert()s are verifiable at compile time.  A check that a\n> static analyzer can verify is more valuable than a run-time check.\n> When a compile-time check is not possible, you'd have to fall back to\n> BUG_ON().\n\nLinux has BUILT_BUG_ON as well, which we may desire?\n"},{"id":"333348","messageId":"20171122232457.GA8577@sigill","threadId":"47304","inReplyTo":"20171122223827.26773-1-sbeller@google.com","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T23:24:57Z","receivedAt":"2017-11-22T23:25:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 22, 2017 at 02:38:24PM -0800, Stefan Beller wrote:\n\n> On reviewing [1] I wondered why there are so many asserts and wondered\n> if these asserts could have been prevented by a better functionality around\n> bug reporting in our code.\n> \n> Introduce a BUG_ON macro, which is superior to assert() by\n>  * being always there, even when compiled with NDEBUG and\n>  * providind an additional human readable error message, like BUG()\n\nI'm not sure I agree with the aim of the series.\n\nIf people want to compile with NDEBUG, that's their business, I guess.\nI don't see much _point_ in it for Git, since most of our assertions do\nnot respect NDEBUG, and I don't think we tend to assert in expensive\nways anyway.\n\nI do like human readable messages. But sometimes such a message just\nmakes the code harder to read (and to write). E.g., is there any real\nvalue in:\n\n  BUG_ON(!foo, \"called bar() with a foo!\");\n\nover:\n\n  assert(foo);\n\n? The error message you'd get from the latter is rather sparse, but the\nfile and line number information it contains should be enough to find\nthe original source line. And after all, it's not _supposed_ to happen,\nso if it does you're likely going to need to dig into the source anyway.\n\nThe human-readable BUG messages I find useful add some context or\nsummarize the situation. E.g. (pulled from random grepping):\n\n  BUG: color parsing ran out of space\n\nis way better than:\n\n  assert failed: len < 2\n\nLikewise, in this code:\n\n  if (hashmap_put(map, alloc_ref_store_hash_entry(name, refs)))\n\tdie(\"BUG: %s ref_store '%s' initialized twice\", type, name);\n\nwe get a lot of extra information:\n\n  - the type is mentioned\n  - the name variable is dereferenced\n  - the implication of \"initialized twice\" is made clear by the author,\n    which would not be immediately obvious just from seeing the failed\n    call\n\nSo I _like_ good messages, but I also think a lot of assertions don't\nreally lend themselves to good messages. And we should shoot for just\nmaking them easy to read and write.\n\nI also find (as your third patch switches):\n\n  if (!foo)\n\tBUG(\"foo has not been setup\");\n\nmore readable than the BUG_ON() version, if only because it uses\ntraditional control flow. But that may just be because I'm used to it.\nI'm sure kernel folks are used to BUG_ON() at this point, and we'd grow\nused to it, too.\n\n-Peff\n"},{"id":"333349","messageId":"20171122232814.GH11671@aiede.mtv.corp.google.com","threadId":"47304","inReplyTo":"20171122232457.GA8577@sigill","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-22T23:28:14Z","receivedAt":"2017-11-22T23:28:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Wed, Nov 22, 2017 at 02:38:24PM -0800, Stefan Beller wrote:\n\n>> On reviewing [1] I wondered why there are so many asserts and wondered\n>> if these asserts could have been prevented by a better functionality around\n>> bug reporting in our code.\n>>\n>> Introduce a BUG_ON macro, which is superior to assert() by\n>>  * being always there, even when compiled with NDEBUG and\n>>  * providind an additional human readable error message, like BUG()\n>\n> I'm not sure I agree with the aim of the series.\n>\n> If people want to compile with NDEBUG, that's their business, I guess.\n> I don't see much _point_ in it for Git, since most of our assertions do\n> not respect NDEBUG, and I don't think we tend to assert in expensive\n> ways anyway.\n>\n> I do like human readable messages. But sometimes such a message just\n> makes the code harder to read (and to write). E.g., is there any real\n> value in:\n>\n>   BUG_ON(!foo, \"called bar() with a foo!\");\n>\n> over:\n>\n>   assert(foo);\n\nI think you're hinting at wanting\n\n\tBUG_ON(!foo);\n\nwhich is something that the Linux kernel has (and which is not done in\nthis series).\n\n[...]\n> I also find (as your third patch switches):\n>\n>   if (!foo)\n> \tBUG(\"foo has not been setup\");\n>\n> more readable than the BUG_ON() version, if only because it uses\n> traditional control flow.\n\nYes, I think you're right.\n\nThanks,\nJonathan\n"},{"id":"333350","messageId":"20171122233709.GB8577@sigill","threadId":"47304","inReplyTo":"20171122230239.GG11671@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T23:37:09Z","receivedAt":"2017-11-22T23:37:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 22, 2017 at 03:02:39PM -0800, Jonathan Nieder wrote:\n\n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n> >  __attribute__((format (printf, 3, 4))) NORETURN\n> >  void BUG_fl(const char *file, int line, const char *fmt, ...);\n> >  #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)\n> > +#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while (0)\n> >  #else\n> >  __attribute__((format (printf, 1, 2))) NORETURN\n> >  void BUG(const char *fmt, ...);\n> > +\n> > +__attribute__((format (printf, 2, 3)))\n> > +void BUG_ON(int condition, const char *fmt, ...);\n> >  #endif\n> \n> I worry that these definitions are mildly incompatible: the macro\n> accepts anything that can go in an 'if', including pointers, and the\n> function only accepts an int.\n\nI suspect this would cause real latent problems. Doing:\n\n  const char *foo = NULL;\n\n  ...\n  BUG_ON(foo, \"foo was set twice!\");\n  foo = xstrdup(bar);\n\nwould compile fine on modern systems, but issue a pointer-to-int\nconversion warning if you didn't have variadic macros. So most git devs\nwould be unlikely to see it, until the trap springs on some poor soul\nbuilding on ancient AIX or something.\n\nOn the other hand, I wouldn't be surprised if our friends on less-abled\ncompilers see a lot of pointless warnings anyway, and almost certainly\ncan't deal with the equivalent of \"-Werror\".\n\nI'm also not sure which compilers that even encompasses these days.\nMaybe we should add variadic macros to our list of weather-balloon c99\nfeatures to test for.\n\n> Is there a way for the macro to typecheck that its argument is an\n> integer to avoid that?\n\nI couldn't think of one.\n\nIt would be nice to just \"!!condition\" in the non-macro case, but of\ncourse we can't do that automatically without a macro.\n\nYou're asking for the opposite: force the macro version to require an\nint, so that the caller has to remember to do \"!!condition\". I can't\nthink of a way to do that, but I'm also not sure I like the direction,\nas it pushes the effort into the callsite.\n\n-Peff\n"},{"id":"333351","messageId":"20171122233905.GC8577@sigill","threadId":"47304","inReplyTo":"20171122232814.GH11671@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T23:39:05Z","receivedAt":"2017-11-22T23:39:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 22, 2017 at 03:28:14PM -0800, Jonathan Nieder wrote:\n\n> > I do like human readable messages. But sometimes such a message just\n> > makes the code harder to read (and to write). E.g., is there any real\n> > value in:\n> >\n> >   BUG_ON(!foo, \"called bar() with a foo!\");\n> >\n> > over:\n> >\n> >   assert(foo);\n> \n> I think you're hinting at wanting\n> \n> \tBUG_ON(!foo);\n> \n> which is something that the Linux kernel has (and which is not done in\n> this series).\n\nYes. I'd be fine having a single-argument BUG_ON() like that. But then,\nI'm not sure what it's buying us over assert().\n\nI get why the kernel cannot use the default \"dump core and exit\"\nbehavior of assert(), but that's basically what our BUG() does.\n\n-Peff\n"},{"id":"333354","messageId":"20171122234532.GI11671@aiede.mtv.corp.google.com","threadId":"47304","inReplyTo":"20171122233905.GC8577@sigill","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-22T23:45:32Z","receivedAt":"2017-11-22T23:45:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Yes. I'd be fine having a single-argument BUG_ON() like that. But then,\n> I'm not sure what it's buying us over assert().\n\nIt lets you build with NDEBUG.  It also goes through our own die()\nhandler, which means that e.g. the error message gets propagated over\nremote transports.\n\nPlease please please, don't rely on side-effects from assert().  They\nwill cause me to run into pain over time.  This issue alone might be\nworth banning use of assert() in the codebase, if we can't trust\nreviewers to catch problematic examples (fortunately reviewers have\nbeen pretty good about that so far, but banning it would free up their\nattention to focus on other aspects of a patch).\n\nJonathan\n"},{"id":"333356","messageId":"20171122235425.GK11671@aiede.mtv.corp.google.com","threadId":"47304","inReplyTo":"CAGZ79kY7yrgYSM7jLQD3Q4sRpjovXY=HZwU4M-Ai_XRPseda9A@mail.gmail.com","subject":"Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-22T23:54:25Z","receivedAt":"2017-11-22T23:54:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Wed, Nov 22, 2017 at 2:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> In a certain ideal world, the preference would be reversed: you'd want\n>> to use assert() wherever you can and require the compiler to check\n>> that all assert()s are verifiable at compile time.  A check that a\n>> static analyzer can verify is more valuable than a run-time check.\n>> When a compile-time check is not possible, you'd have to fall back to\n>> BUG_ON().\n>\n> Linux has BUILT_BUG_ON as well, which we may desire?\n\nI thought so until I tried to use it:\nhttps://public-inbox.org/git/20170830065631.GH153983@aiede.mtv.corp.google.com/\n\nSo I think we are stuck with run-time checking for now.\n\nThanks,\nJonathan\n"},{"id":"333357","messageId":"20171122235851.GE8577@sigill","threadId":"47304","inReplyTo":"20171122234532.GI11671@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T23:58:51Z","receivedAt":"2017-11-22T23:58:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Yes. I'd be fine having a single-argument BUG_ON() like that. But then,\n> > I'm not sure what it's buying us over assert().\n> \n> It lets you build with NDEBUG.\n\nBut why do you want to build with NDEBUG if nothing uses assert()? ;)\n\nI'm being a little glib, but I think there really is a chicken-and-egg\nquestion here. Why are people building with NDEBUG if they don't want to\ndisable asserts? Do they do it out of habit? A misguided sense of\nperformance optimization? Does it do something else I'm not aware of?\n\n> It also goes through our own die()\n> handler, which means that e.g. the error message gets propagated over\n> remote transports.\n\nBUG() doesn't go through our die handler. It hits vreportf(), which does\ndo some minor munging, but it always goes to stderr. Error message\npropagation over the remote protocol happens either:\n\n  1. From a parent process muxing our stderr onto the sideband.\n\n  2. Via special wrappers like receive-pack's rp_error().\n\nThe only program I know that sets a custom die handler to change the\nreporting is git-http-backend (and there it only applies to die(\"BUG\"),\nnot BUG(), so with the latter you'd get a hard hangup instead of an\nattempt at an http 500).\n\n> Please please please, don't rely on side-effects from assert().  They\n> will cause me to run into pain over time.  This issue alone might be\n> worth banning use of assert() in the codebase, if we can't trust\n> reviewers to catch problematic examples (fortunately reviewers have\n> been pretty good about that so far, but banning it would free up their\n> attention to focus on other aspects of a patch).\n\nYes, obviously side effects in an assert() are horrible. But I haven't\nnoticed that being a problem in our code base.\n\nFor the record, I'm totally fine with banning assert() in favor of a\ncustom equivalent. I just don't think we've seen any real problems with\nassert in our codebase so far.\n\n-Peff\n"},{"id":"333359","messageId":"20171123000839.GL11671@aiede.mtv.corp.google.com","threadId":"47304","inReplyTo":"20171122235851.GE8577@sigill","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-23T00:08:39Z","receivedAt":"2017-11-23T00:08:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:\n\n>> It lets you build with NDEBUG.\n>\n> But why do you want to build with NDEBUG if nothing uses assert()? ;)\n\nNo idea, but some distros (not Debian) have done it before and I don't\nwant to be burned again.\n\n> I'm being a little glib, but I think there really is a chicken-and-egg\n> question here. Why are people building with NDEBUG if they don't want to\n> disable asserts?\n\nIt's because they want to disable asserts.\n\nThe semantics of assert is that they're allowed to be compiled out.  A\nperson setting NDEBUG is perfectly within their rights to assume that\nthe author of a program has followed those semantics.  Of course this\nmakes assert a terrible API from the point of view of Rusty Russel's\nAPI rating scheme\n<http://sweng.the-davies.net/Home/rustys-api-design-manifesto> but\nit's what C gives us.\n\nFWIW I think we've done fine at using assert so far.  But if I\nunderstand correctly, the point of this series is to stop having to\nworry about it.\n\n[...]\n>> It also goes through our own die()\n>> handler, which means that e.g. the error message gets propagated over\n>> remote transports.\n>\n> BUG() doesn't go through our die handler. It hits vreportf(), which does\n> do some minor munging, but it always goes to stderr. Error message\n> propagation over the remote protocol happens either:\n>\n>   1. From a parent process muxing our stderr onto the sideband.\n>\n>   2. Via special wrappers like receive-pack's rp_error().\n>\n> The only program I know that sets a custom die handler to change the\n> reporting is git-http-backend (and there it only applies to die(\"BUG\"),\n> not BUG(), so with the latter you'd get a hard hangup instead of an\n> attempt at an http 500).\n\nAh, interesting.  That could be worth changing, though I don't think\nit's too important.\n\n[...]\n> Yes, obviously side effects in an assert() are horrible. But I haven't\n> noticed that being a problem in our code base.\n>\n> For the record, I'm totally fine with banning assert() in favor of a\n> custom equivalent. I just don't think we've seen any real problems with\n> assert in our codebase so far.\n\nIt sounds like we basically agree, then. :)\n\nJonathan\n"},{"id":"333360","messageId":"20171123001048.GA9465@sigill","threadId":"47304","inReplyTo":"20171123000839.GL11671@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-23T00:10:48Z","receivedAt":"2017-11-23T00:10:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 22, 2017 at 04:08:39PM -0800, Jonathan Nieder wrote:\n\n> > For the record, I'm totally fine with banning assert() in favor of a\n> > custom equivalent. I just don't think we've seen any real problems with\n> > assert in our codebase so far.\n> \n> It sounds like we basically agree, then. :)\n\nYeah, I didn't quite understand your argument at first. If it's that\nasserts are potentially dangerous, I buy that. They haven't bit us, but\nthey could.\n\nThat would be a good thing for Stefan to put in his commit message. ;)\n\n-Peff\n"},{"id":"333363","messageId":"xmqqefopsrk0.fsf@gitster.mtv.corp.google.com","threadId":"47304","inReplyTo":"20171123000839.GL11671@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-23T01:38:07Z","receivedAt":"2017-11-23T01:38:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> FWIW I think we've done fine at using assert so far.  But if I\n> understand correctly, the point of this series is to stop having to\n> worry about it.\n\nI recalled that there was at least one, and \"log -Sassert\" piped to\n\"less\" that looks for \"/^[ ^I]*assert\\(\" caught me one recent one.\n\n    08414938 (\"mailinfo.c: move side-effects outside of assert\", 2016-12-19)\n\nEven though I do not personally mind\n\n\tassert(flags & EXPECTED_BIT);\n\tassert(remaining_doshes == 0);\n\nleft as a reminder primarily for coders, we can do just as well do\nso with\n\n\tif (remaining_doshes != 0)\n\t\tBUG(\"the gostak did not distim all doshes???\");\n\nSo I am fine if we want to move to reduce the use of assert()s or\nget rid of them.  I personally prefer (like Peff, if I am not\nmistaken) an explicit use of the usual control structure, as it is\neasy to follow.  BUG_ON() would become another thing readers need to\nget used to, if we were to use it, and my gut feeling is that it may\nnot be worth it.\n\nA few more random things related to this topic that comes to my\nmind:\n\n - If we had a good set of tools to tell us if an expression is free\n   of side-effects, then assert(<expression>) would be less\n   problematic---we could mechanically check if an assert() that is\n   left as a reminder for coders/readers is safe.\n\n - Even if we had such a check, using the check only on new changes\n   when a patch is accepted is not good enough.  An assert(distim())\n   may have been safe back when it was added because distim() used\n   to be free of side-effects, but a later update to it may add side\n   effects to it.\n\n - The issue that is caused by \"this function used to be pure but\n   lately it gained side-effects\" is not limited to assert().  Using\n   it in \"if (condition) BUG(...)\" or \"BUG_ON(condition,...)\" will\n   not sidestep the fact that such a change will alter behaviour of\n   callers of the function.  It's just that assert(condition) is\n   conditionally compiled, which makes the issue a worse one.\n\n\n"},{"id":"333372","messageId":"20171123050009.GA22218@sigill","threadId":"47304","inReplyTo":"xmqqefopsrk0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-23T05:00:09Z","receivedAt":"2017-11-23T05:00:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 23, 2017 at 10:38:07AM +0900, Junio C Hamano wrote:\n\n> > FWIW I think we've done fine at using assert so far.  But if I\n> > understand correctly, the point of this series is to stop having to\n> > worry about it.\n> \n> I recalled that there was at least one, and \"log -Sassert\" piped to\n> \"less\" that looks for \"/^[ ^I]*assert\\(\" caught me one recent one.\n> \n>     08414938 (\"mailinfo.c: move side-effects outside of assert\", 2016-12-19)\n\nThanks, I forgot about that one. There's some discussion about NDEBUG in\nthe surrounding thread if anybody is interested:\n\n  https://public-inbox.org/git/900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0/\n\n(but it's long and there's really no resolution, so you may want to skip\nit).\n\n> Even though I do not personally mind\n> \n> \tassert(flags & EXPECTED_BIT);\n> \tassert(remaining_doshes == 0);\n> \n> left as a reminder primarily for coders, we can do just as well do\n> so with\n> \n> \tif (remaining_doshes != 0)\n> \t\tBUG(\"the gostak did not distim all doshes???\");\n\nYeah, agreed. The reason I do not mind the assert() form is that if you\nhave nothing useful to say in the BUG() sentence, it's a bit more\ncompact.\n\n> So I am fine if we want to move to reduce the use of assert()s or\n> get rid of them.  I personally prefer (like Peff, if I am not\n> mistaken) an explicit use of the usual control structure, as it is\n> easy to follow.\n\nTo clarify my position, I think BUG_ON(cond, msg) from this series\nprovides basically no value over \"if (cond) BUG(msg)\". But I could see\nvalue in \"BUG_ON(cond)\" that allows the compact form but doesn't respect\nNDEBUG.\n\n-Peff\n"}]}