[PATCH 1/5] minor whitespace cleanups

classic Classic list List threaded Threaded
12 messages Options
Piotr Trojanek Piotr Trojanek
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/5] minor whitespace cleanups

---
 tag-util.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git tag-util.c tag-util.c
index 343c161f..7091d294 100644
--- tag-util.c
+++ tag-util.c
@@ -34,7 +34,6 @@ line_error (tag_parse_status_t status,
 const char *
 illegal_tag (const char *tag, notmuch_bool_t remove)
 {
-
     if (*tag == '\0' && ! remove)
  return "empty tag forbidden";
 
@@ -155,7 +154,6 @@ tag_parse_status_t
 parse_tag_command_line (void *ctx, int argc, char **argv,
  char **query_str, tag_op_list_t *tag_ops)
 {
-
     int i;
 
     for (i = 0; i < argc; i++) {
@@ -209,7 +207,6 @@ makes_changes (notmuch_message_t *message,
        tag_op_list_t *list,
        tag_op_flag_t flags)
 {
-
     size_t i;
 
     notmuch_tags_t *tags;
--
2.11.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Piotr Trojanek Piotr Trojanek
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/5] add leaks due to missing invocations of va_end

As the Linux man page states: "Each invocation of va_start() must be
matched by a corresponding invocation of va_end() in the same
function." Detected by cppcheck.
---
 tag-util.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git tag-util.c tag-util.c
index 7091d294..30c2c077 100644
--- tag-util.c
+++ tag-util.c
@@ -28,6 +28,9 @@ line_error (tag_parse_status_t status,
     fprintf (stderr, status < 0 ? "Error: " : "Warning: ");
     vfprintf (stderr, format, va_args);
     fprintf (stderr, " [%s]\n", line);
+
+    va_end (va_args);
+
     return status;
 }
 
@@ -200,6 +203,8 @@ message_error (notmuch_message_t *message,
     vfprintf (stderr, format, va_args);
     fprintf (stderr, "Message-ID: %s\n", notmuch_message_get_message_id (message));
     fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
+
+    va_end (va_args);
 }
 
 static int
--
2.11.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Piotr Trojanek Piotr Trojanek
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/5] remove ineffective assignments

In reply to this post by Piotr Trojanek
Detected by cppecheck.
---
 notmuch-new.c | 2 +-
 tag-util.c    | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git notmuch-new.c notmuch-new.c
index e2822e23..4d40f3d0 100644
--- notmuch-new.c
+++ notmuch-new.c
@@ -850,7 +850,7 @@ _remove_directory (void *ctx,
    const char *path,
    add_files_state_t *add_files_state)
 {
-    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+    notmuch_status_t status;
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
     char *absolute;
diff --git tag-util.c tag-util.c
index 30c2c077..d9fca7b8 100644
--- tag-util.c
+++ tag-util.c
@@ -218,7 +218,6 @@ makes_changes (notmuch_message_t *message,
     notmuch_bool_t changes = FALSE;
 
     /* First, do we delete an existing tag? */
-    changes = FALSE;
     for (tags = notmuch_message_get_tags (message);
  ! changes && notmuch_tags_valid (tags);
  notmuch_tags_move_to_next (tags)) {
--
2.11.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Piotr Trojanek Piotr Trojanek
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

In reply to this post by Piotr Trojanek
---
 notmuch-count.c | 2 +-
 notmuch-new.c   | 4 ++--
 notmuch-reply.c | 2 +-
 notmuch-show.c  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git notmuch-count.c notmuch-count.c
index a05b430d..50b0c193 100644
--- notmuch-count.c
+++ notmuch-count.c
@@ -111,7 +111,7 @@ print_count (notmuch_database_t *notmuch, const char *query_str,
     case OUTPUT_FILES:
  count = count_files (query);
  if (count >= 0) {
-    printf ("%u", count);
+    printf ("%d", count);
  } else {
     ret = -1;
     goto DONE;
diff --git notmuch-new.c notmuch-new.c
index 4d40f3d0..3a60f7ca 100644
--- notmuch-new.c
+++ notmuch-new.c
@@ -131,10 +131,10 @@ generic_print_progress (const char *action, const char *object,
     elapsed_overall = notmuch_time_elapsed (tv_start, tv_now);
     rate_overall = processed / elapsed_overall;
 
-    printf ("%s %d ", action, processed);
+    printf ("%s %u ", action, processed);
 
     if (total) {
- printf ("of %d %s", total, object);
+ printf ("of %u %s", total, object);
  if (processed > 0 && elapsed_overall > 0.5) {
     double time_remaining = ((total - processed) / rate_overall);
     printf (" (");
diff --git notmuch-reply.c notmuch-reply.c
index b88f1d31..e6c16641 100644
--- notmuch-reply.c
+++ notmuch-reply.c
@@ -635,7 +635,7 @@ static int do_reply(notmuch_config_t *config,
     return 1;
 
  if (count != 1) {
-    fprintf (stderr, "Error: search term did not match precisely one message (matched %d messages).\n", count);
+    fprintf (stderr, "Error: search term did not match precisely one message (matched %u messages).\n", count);
     return 1;
  }
 
diff --git notmuch-show.c notmuch-show.c
index accea48a..3ce4b63c 100644
--- notmuch-show.c
+++ notmuch-show.c
@@ -902,7 +902,7 @@ do_show_single (void *ctx,
  return 1;
 
     if (count != 1) {
- fprintf (stderr, "Error: search term did not match precisely one message (matched %d messages).\n", count);
+ fprintf (stderr, "Error: search term did not match precisely one message (matched %u messages).\n", count);
  return 1;
     }
 
--
2.11.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Piotr Trojanek Piotr Trojanek
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/5] flag potential problems with FIXME

In reply to this post by Piotr Trojanek
Detected by cppcheck.
---
 lib/directory.cc | 1 +
 lib/string-map.c | 1 +
 2 files changed, 2 insertions(+)

diff --git lib/directory.cc lib/directory.cc
index 5de3319c..801f1e5b 100644
--- lib/directory.cc
+++ lib/directory.cc
@@ -302,6 +302,7 @@ notmuch_directory_delete (notmuch_directory_t *directory)
        "A Xapian exception occurred deleting directory entry: %s.\n",
        error.get_msg().c_str());
  directory->notmuch->exception_reported = TRUE;
+ /* FIXME: Variable 'status' is assigned a value that is never used. */
  status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
     notmuch_directory_destroy (directory);
diff --git lib/string-map.c lib/string-map.c
index 0bb77e93..11c0a644 100644
--- lib/string-map.c
+++ lib/string-map.c
@@ -122,6 +122,7 @@ bsearch_first (notmuch_string_pair_t *array, size_t len, const char *key, notmuc
     size_t last = len - 1;
     size_t mid;
 
+    /* FIXME: Checking if unsigned variable 'len' is less than zero. */
     if (len <= 0)
  return NULL;
 
--
2.11.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

In reply to this post by Piotr Trojanek
this series up to patch 4 LGTM, and seem uncontroversial.  patch 5 adds
FIXMEs that should probably actually be fixed, though, rather than just
flagged.

Thanks to Piotr for identifying these issues and suggesting the
cleanup.

         --dkg
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/5] minor whitespace cleanups

In reply to this post by Piotr Trojanek
Piotr Trojanek <[hidden email]> writes:

> ---
>  tag-util.c | 3 ---
>  1 file changed, 3 deletions(-)

btw, please don't use -p0 for future notmuch patches, it confuses my
tools.

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

In reply to this post by Piotr Trojanek

pushed the first 4 to master.

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Piotr Trojanek Piotr Trojanek
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

In reply to this post by Daniel Kahn Gillmor
On Wed, Jun 21, 2017 at 5:11 PM, Daniel Kahn Gillmor
<[hidden email]> wrote:
> patch 5 adds FIXMEs that should probably actually be fixed, though, rather than just flagged.

Thanks for merging the uncontroversial patches. Fixing the flagged
problems is not obvious to me, it really depends on your intentions.

For the first FIXME, the documentation for notmuch_directory_delete
says (lib/notmuch.h:1971):

 * Delete directory document from the database, and destroy the
 * notmuch_directory_t object.

but that is not what happens, for example, if the call to
_notmuch_database_ensure_writable fails. Then the notmuch_directory_t
object is only destroyed by the caller (see the end of
_remove_directory function, notmuch-new.c:886).

The comment should clearly say if the object is always destroyed or
only if no error happened.

For the second FIXME, I don't quite see why not just use the bsearch
function. It could be called either with strcmp (if exact is true) or
with a simple wrapper around strncmp (if exact is false). This wrapper
could replace the string_cmp routine, so together with bsearch this
could even make the code smaller.

Also, I don't really understand the intention behind declaring
string_cmp as returning notmuch_bool_t and then, in bsearch_first,
casting its result to int.

Am I missing something?

--
Piotr Trojanek
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

Piotr Trojanek <[hidden email]> writes:

> For the second FIXME, I don't quite see why not just use the bsearch
> function. It could be called either with strcmp (if exact is true) or
> with a simple wrapper around strncmp (if exact is false). This wrapper
> could replace the string_cmp routine, so together with bsearch this
> could even make the code smaller.

AFAIK, bsearch does not guarantee to return the first string matching
the key, which is what we need here.

>
> Also, I don't really understand the intention behind declaring
> string_cmp as returning notmuch_bool_t and then, in bsearch_first,
> casting its result to int.

yes, that looks odd to me also, especially since it really just wraps
strcmp / strncmp, which are signed. Probably just an error on my part.

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

In reply to this post by Piotr Trojanek
Piotr Trojanek <[hidden email]> writes:

> On Wed, Jun 21, 2017 at 5:11 PM, Daniel Kahn Gillmor
> <[hidden email]> wrote:
>> patch 5 adds FIXMEs that should probably actually be fixed, though, rather than just flagged.
>
> Thanks for merging the uncontroversial patches. Fixing the flagged
> problems is not obvious to me, it really depends on your intentions.
>
> For the first FIXME, the documentation for notmuch_directory_delete
> says (lib/notmuch.h:1971):
>
>  * Delete directory document from the database, and destroy the
>  * notmuch_directory_t object.
>
> but that is not what happens, for example, if the call to
> _notmuch_database_ensure_writable fails. Then the notmuch_directory_t
> object is only destroyed by the caller (see the end of
> _remove_directory function, notmuch-new.c:886).
>
> The comment should clearly say if the object is always destroyed or
> only if no error happened.

It seems like it would be cleaner to do something like the patch below,
then amend the comment to say the notmuch_directory_t object is
destroyed only on successful deletion of the document.


diff --git a/lib/directory.cc b/lib/directory.cc
index 5de3319c..d8f7763b 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -297,6 +297,7 @@ notmuch_directory_delete (notmuch_directory_t *directory)
     try {
        db = static_cast <Xapian::WritableDatabase *> (directory->notmuch->xapian_db);
        db->delete_document (directory->document_id);
+       notmuch_directory_destroy (directory);
     } catch (const Xapian::Error &error) {
        _notmuch_database_log (directory->notmuch,
                               "A Xapian exception occurred deleting directory entry: %s.\n",
@@ -304,9 +305,8 @@ notmuch_directory_delete (notmuch_directory_t *directory)
        directory->notmuch->exception_reported = TRUE;
        status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
-    notmuch_directory_destroy (directory);
 
-    return NOTMUCH_STATUS_SUCCESS;
+    return status;
 }
 
 void
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Jani Nikula Jani Nikula
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/5] remove ineffective assignments

In reply to this post by Piotr Trojanek
On Fri, 16 Jun 2017, Piotr Trojanek <[hidden email]> wrote:
> Detected by cppecheck.

As I just commented about fixing shellcheck issues in
emacs/notmuch-emacs-mua, I'd really appreciate a build target to run the
static checkers, if available, and have them run as part of build (with
some checker option), release checks, make test, or as a separate
target. Otherwise the same errors creep right back in.

BR,
Jani.



> ---
>  notmuch-new.c | 2 +-
>  tag-util.c    | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git notmuch-new.c notmuch-new.c
> index e2822e23..4d40f3d0 100644
> --- notmuch-new.c
> +++ notmuch-new.c
> @@ -850,7 +850,7 @@ _remove_directory (void *ctx,
>     const char *path,
>     add_files_state_t *add_files_state)
>  {
> -    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> +    notmuch_status_t status;
>      notmuch_directory_t *directory;
>      notmuch_filenames_t *files, *subdirs;
>      char *absolute;
> diff --git tag-util.c tag-util.c
> index 30c2c077..d9fca7b8 100644
> --- tag-util.c
> +++ tag-util.c
> @@ -218,7 +218,6 @@ makes_changes (notmuch_message_t *message,
>      notmuch_bool_t changes = FALSE;
>  
>      /* First, do we delete an existing tag? */
> -    changes = FALSE;
>      for (tags = notmuch_message_get_tags (message);
>   ! changes && notmuch_tags_valid (tags);
>   notmuch_tags_move_to_next (tags)) {
> --
> 2.11.0
>
> _______________________________________________
> notmuch mailing list
> [hidden email]
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch