Skip to content

Commit

Permalink
Use "g_return_val_if_fail()" instead of producing an own error message.
Browse files Browse the repository at this point in the history
  • Loading branch information
@lpaulsen93
lpaulsen93 committed Aug 19, 2017
1 parent 00b8941 commit 109b8d0
Showing 1 changed file with 125 additions and 141 deletions.
266 changes: 125 additions & 141 deletions workbench/src/wb_project.c
View file
Expand Up @@ -1239,74 +1239,66 @@ guint wb_project_get_bookmarks_count(WB_PROJECT *prj)
**/
gboolean wb_project_save(WB_PROJECT *prj, GError **error)
{
GKeyFile *kf;
guint index;
gchar *contents;
gsize length, boomarks_size;
gboolean success = FALSE;
WB_PROJECT_ON_SAVE_USER_DATA tmp;

if (prj != NULL)
g_return_val_if_fail(prj, FALSE);

This comment has been minimized.

Sign in to view

Copy link
@elextr

elextr Aug 19, 2017

Member

Are you aware that its possible for distributions or others to build with G_DISABLE_CHECKS and the g_return_val_if_failed will be removed, so the function will crash on some poor user if their actions happen to result in the function being called with a NULL?

Your documentation for the function doesn't disallow NULL, so you always need to reject it. Probably better to use an if and g_warning or g_critical` for the message, which can be made to crash for easier debugging by setting G_DEBUG.

There have been some spirited Geany discussions about the "right" way to use these Glib debugging macros (IMHO there is none if the macro can be compiled out, unless you can prove by analysis or exhaustive testing that the failure cannot occur to the above mentioned innocent user, but if you can do that you don't need the test in the first place). Its certainly the case that Geany itself is still subject to the problem referred to above.

This comment has been minimized.

Sign in to view

Copy link
@lpaulsen93

lpaulsen93 Aug 19, 2017

Author Contributor

Hmmm...yes I have also seen some calls like mine in other plugins. Let's say it this way: if never ever a programming error occurs than it is useless to check for NULL in this case. But whom am I to think that there never will be errors in the future? So I would always check pointers on the "boundaries" functions and just maybe not on internal static functions. In the case of an error it still prevents the user from a crash.

But for now I will keep the code like that. What about writing some own Geany debugging macros which use glib macros if compiled in and if not still guarantee an defined action and consistent error output among geany and geany plugins?

This comment has been minimized.

Sign in to view

Copy link
@b4n

b4n Aug 19, 2017

Member

@elextr I find it pertinent to use these macros when they are meant to detect programming errors. Yes, you can be forcefully defensive and always check no matter the debugging status, but the point these macros can be disabled (although in practice most builders don't disable them anyway) is for one to be able to trade safety for more speed.

So IMO if a builder do want to disable those checks, fine, but they gotta know it's less defensive. Which could make sense, as these checks are for programming errors, and should not depend on user behavior.

In all cases, I think using a GError for programming error is worse than emitting a warning somewhere, because it's harder to handle the error and it could be ignored and left unnoticed more easily.

Your documentation for the function doesn't disallow NULL

It odes, unless you accept NULL to be valid for The project, and as the function is supposed to save it to a file, I wouldn't expect NULL to be a valid one unless explicitly stated otherwise.

This comment has been minimized.

Sign in to view

Copy link
@elextr

elextr Aug 20, 2017

Member

@b4n

(although in practice most builders don't disable them anyway)

Because they know things will crash if they do 😦

as these checks are for programming errors, and should not depend on user behavior.

Thats a specious argument. Since most programmers fix crashing errors that occur every time the program runs, the errors MUST depend on user input otherwise they would be fixed if they occurred all the time. So unless you apply analysis or exhaustive testing user input can always expose programming errors, so if the program can politely decline to perform the user operation and keep running its better than crashing.

This comment has been minimized.

Sign in to view

Copy link
@codebrainz

codebrainz Aug 20, 2017

Member

Because they know things will crash if they do

They will crash or do whatever undefined behaviour either way.

Thats a specious argument. Since most programmers fix crashing errors that occur every time the program runs, the errors MUST depend on user input otherwise they would be fixed if they occurred all the time. So unless you apply analysis or exhaustive testing user input can always expose programming errors, so if the program can politely decline to perform the user operation and keep running its better than crashing.

No, these assertions are meant only for programming error, using them for release-build control flow is not advised. See from the docs below:

The g_return family of macros (g_return_if_fail(), g_return_val_if_fail(), g_return_if_reached(), g_return_val_if_reached()) should only be used for programming errors, a typical use case is checking for invalid parameters at the beginning of a public function. They should not be used if you just mean "if (error) return", they should only be used if you mean "if (bug in program) return". The program behavior is generally considered undefined after one of these checks fails. They are not intended for normal control flow, only to give a perhaps-helpful warning before giving up.

This comment has been minimized.

Sign in to view

Copy link
@elextr

elextr Aug 20, 2017

Member

@codebrainz @b4n, its probably best we take this argument somewhere else, not hijack another pull request 😁

Apologies to @LarsGit223

This comment has been minimized.

Sign in to view

Copy link
@lpaulsen93

lpaulsen93 Aug 20, 2017

Author Contributor

😄 No problem. Maybe we should open a new ticket as a discussion of this point. The goal for me would be to find a common way that every one can agree to and probably could become part of the coding guidelines.

/* Load existing data into GKeyFile */
kf = g_key_file_new ();
if (!g_key_file_load_from_file(kf, prj->filename, G_KEY_FILE_NONE, error))
{
GKeyFile *kf;
guint index;
gchar *contents;
gsize length, boomarks_size;
return FALSE;
}

/* Load existing data into GKeyFile */
kf = g_key_file_new ();
if (!g_key_file_load_from_file(kf, prj->filename, G_KEY_FILE_NONE, error))
{
return FALSE;
}
/* Remove existing, old data from our plugin */
g_key_file_remove_group (kf, "Workbench", NULL);

/* Remove existing, old data from our plugin */
g_key_file_remove_group (kf, "Workbench", NULL);
/* Save Project bookmarks as string list */
boomarks_size = wb_project_get_bookmarks_count(prj);
if (boomarks_size > 0)
{
gchar **bookmarks_strings, *file, *rel_path;

/* Save Project bookmarks as string list */
boomarks_size = wb_project_get_bookmarks_count(prj);
if (boomarks_size > 0)
bookmarks_strings = g_new0(gchar *, boomarks_size+1);
for (index = 0 ; index < boomarks_size ; index++ )
{
gchar **bookmarks_strings, *file, *rel_path;
file = wb_project_get_bookmark_at_index(prj, index);
rel_path = get_any_relative_path(prj->filename, file);

bookmarks_strings = g_new0(gchar *, boomarks_size+1);
for (index = 0 ; index < boomarks_size ; index++ )
{
file = wb_project_get_bookmark_at_index(prj, index);
rel_path = get_any_relative_path(prj->filename, file);

bookmarks_strings[index] = rel_path;
}
g_key_file_set_string_list
(kf, "Workbench", "Bookmarks", (const gchar **)bookmarks_strings, boomarks_size);
for (index = 0 ; index < boomarks_size ; index++ )
{
g_free (bookmarks_strings[index]);
}
g_free(bookmarks_strings);
bookmarks_strings[index] = rel_path;
}
g_key_file_set_string_list
(kf, "Workbench", "Bookmarks", (const gchar **)bookmarks_strings, boomarks_size);
for (index = 0 ; index < boomarks_size ; index++ )
{
g_free (bookmarks_strings[index]);
}
g_free(bookmarks_strings);
}

/* Init tmp data */
tmp.kf = kf;
tmp.dir_count = 1;
/* Init tmp data */
tmp.kf = kf;
tmp.dir_count = 1;

/* Store our directories */
g_slist_foreach(prj->directories, (GFunc)wb_project_save_directories, &tmp);
/* Store our directories */
g_slist_foreach(prj->directories, (GFunc)wb_project_save_directories, &tmp);

/* Get data as string */
contents = g_key_file_to_data (kf, &length, error);
g_key_file_free(kf);
/* Get data as string */
contents = g_key_file_to_data (kf, &length, error);
g_key_file_free(kf);

/* Save to file */
success = g_file_set_contents (prj->filename, contents, length, error);
if (success)
{
prj->modified = FALSE;
}
g_free (contents);
}
else if (error != NULL)
/* Save to file */
success = g_file_set_contents (prj->filename, contents, length, error);
if (success)
{
g_set_error (error, 0, 0,
"Internal error: param missing (file: %s, line %d)",
__FILE__, __LINE__);
prj->modified = FALSE;
}
g_free (contents);

return success;
}
Expand All @@ -1325,118 +1317,110 @@ gboolean wb_project_save(WB_PROJECT *prj, GError **error)
**/
gboolean wb_project_load(WB_PROJECT *prj, gchar *filename, GError **error)
{
GKeyFile *kf;
guint index;
gchar *contents;
gchar key[100];
gsize length;
gboolean success = FALSE;

if (prj != NULL)
g_return_val_if_fail(prj, FALSE);

if (!g_file_get_contents (filename, &contents, &length, error))
{
GKeyFile *kf;
guint index;
gchar *contents;
gchar key[100];
gsize length;
return FALSE;
}

if (!g_file_get_contents (filename, &contents, &length, error))
{
return FALSE;
}
kf = g_key_file_new ();

kf = g_key_file_new ();
if (!g_key_file_load_from_data (kf, contents, length,
G_KEY_FILE_KEEP_COMMENTS | G_KEY_FILE_KEEP_TRANSLATIONS,
error))
{
g_key_file_free (kf);
g_free (contents);
return FALSE;
}

if (!g_key_file_load_from_data (kf, contents, length,
G_KEY_FILE_KEEP_COMMENTS | G_KEY_FILE_KEEP_TRANSLATIONS,
error))
{
g_key_file_free (kf);
g_free (contents);
return FALSE;
}
if (g_key_file_has_group (kf, "Workbench"))
{
WB_PROJECT_DIR *new_dir;
gchar *str;
gchar **splitv, **bookmarks_strings;

if (g_key_file_has_group (kf, "Workbench"))
/* Load project bookmarks from string list */
bookmarks_strings = g_key_file_get_string_list (kf, "Workbench", "Bookmarks", NULL, error);
if (bookmarks_strings != NULL)
{
WB_PROJECT_DIR *new_dir;
gchar *str;
gchar **splitv, **bookmarks_strings;
gchar **file, *abs_path;

/* Load project bookmarks from string list */
bookmarks_strings = g_key_file_get_string_list (kf, "Workbench", "Bookmarks", NULL, error);
if (bookmarks_strings != NULL)
file = bookmarks_strings;
while (*file != NULL)
{
gchar **file, *abs_path;

file = bookmarks_strings;
while (*file != NULL)
abs_path = get_combined_path(prj->filename, *file);
if (abs_path != NULL)
{
abs_path = get_combined_path(prj->filename, *file);
if (abs_path != NULL)
{
wb_project_add_bookmark_int(prj, abs_path);
g_free(abs_path);
}
file++;
wb_project_add_bookmark_int(prj, abs_path);
g_free(abs_path);
}
g_strfreev(bookmarks_strings);
file++;
}
g_strfreev(bookmarks_strings);
}

/* Load project dirs */
for (index = 1 ; index < 1025 ; index++)
/* Load project dirs */
for (index = 1 ; index < 1025 ; index++)
{
g_snprintf(key, sizeof(key), "Dir%u-BaseDir", index);
if (!g_key_file_has_key (kf, "Workbench", key, NULL))
{
g_snprintf(key, sizeof(key), "Dir%u-BaseDir", index);
if (!g_key_file_has_key (kf, "Workbench", key, NULL))
{
break;
}
break;
}

str = g_key_file_get_string(kf, "Workbench", key, NULL);
if (str == NULL)
{
break;
}
new_dir = wb_project_add_directory_int(prj, str, FALSE);
if (new_dir == NULL)
{
break;
}
str = g_key_file_get_string(kf, "Workbench", key, NULL);
if (str == NULL)
{
break;
}
new_dir = wb_project_add_directory_int(prj, str, FALSE);
if (new_dir == NULL)
{
break;
}

g_snprintf(key, sizeof(key), "Dir%u-FilePatterns", index);
str = g_key_file_get_string(kf, "Workbench", key, NULL);
if (str != NULL)
{
splitv = g_strsplit (str, ";", -1);
wb_project_dir_set_file_patterns(new_dir, splitv);
}
g_free(str);
g_snprintf(key, sizeof(key), "Dir%u-FilePatterns", index);
str = g_key_file_get_string(kf, "Workbench", key, NULL);
if (str != NULL)
{
splitv = g_strsplit (str, ";", -1);
wb_project_dir_set_file_patterns(new_dir, splitv);
}
g_free(str);

g_snprintf(key, sizeof(key), "Dir%u-IgnoredDirsPatterns", index);
str = g_key_file_get_string(kf, "Workbench", key, NULL);
if (str != NULL)
{
splitv = g_strsplit (str, ";", -1);
wb_project_dir_set_ignored_dirs_patterns(new_dir, splitv);
}
g_free(str);
g_snprintf(key, sizeof(key), "Dir%u-IgnoredDirsPatterns", index);
str = g_key_file_get_string(kf, "Workbench", key, NULL);
if (str != NULL)
{
splitv = g_strsplit (str, ";", -1);
wb_project_dir_set_ignored_dirs_patterns(new_dir, splitv);
}
g_free(str);

g_snprintf(key, sizeof(key), "Dir%u-IgnoredFilePatterns", index);
str = g_key_file_get_string(kf, "Workbench", key, NULL);
if (str != NULL)
{
splitv = g_strsplit (str, ";", -1);
wb_project_dir_set_ignored_file_patterns(new_dir, splitv);
}
g_free(str);
g_snprintf(key, sizeof(key), "Dir%u-IgnoredFilePatterns", index);
str = g_key_file_get_string(kf, "Workbench", key, NULL);
if (str != NULL)
{
splitv = g_strsplit (str, ";", -1);
wb_project_dir_set_ignored_file_patterns(new_dir, splitv);
}
g_free(str);
}

g_key_file_free(kf);
g_free (contents);
success = TRUE;
}
else if (error != NULL)
{
g_set_error (error, 0, 0,
"Internal error: param missing (file: %s, line %d)",
__FILE__, __LINE__);
}

g_key_file_free(kf);
g_free (contents);
success = TRUE;

return success;
}

Expand Down

0 comments on commit 109b8d0

Please sign in to comment.