Skip to content
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

lineoperations: fixed -Werror=aggregate-return #517

Merged
merged 3 commits into from
Jul 2, 2019

Conversation

smostertdev
Copy link
Contributor

Oops, I think I broke Travis CI.

I am returning a (small) structure when I should be using pointers.
This commit should fix this mess, my apologies.

@codebrainz
Copy link
Member

Weird that Travis breaks for perfectly valid code.

@techee
Copy link
Member

techee commented Feb 18, 2017

I think -Werror=aggregate-return is too aggressive and shouldn't be used. I run into the same problem in geany/geany#1263 where some some functions in ctags return MIOPos which is a tiny structure. I'll prepare a patch to remove this flag from the build - this pull request will then be unnecessary.

@techee
Copy link
Member

techee commented Feb 18, 2017

Created #529. Depending whether it's OK the first patch from this pull request could be dropped. The second is still necessary as build fails without it.

Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from the unnecessary allocations.

Also, I'm fine with depending on @techee's PR, so you can drop the first commit if you want.

gint num_chars = 0;
gint i = 0;
gint lines_affected = 0;

struct lo_lines *sel = g_malloc(sizeof(struct lo_lines));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of allocating on the heap you could pass &sel to the get_current_sel_lines() call below

@@ -223,18 +227,26 @@ action_sci_manip_item(GtkMenuItem *menuitem, gpointer gdata)
/* function pointer to gdata -- function to be used */
gint (*func)(ScintillaObject *, gint, gint) = gdata;
GeanyDocument *doc = document_get_current();
struct lo_lines sel = get_current_sel_lines(doc->editor->sci);

struct lo_lines *sel = g_malloc(sizeof(struct lo_lines));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@smostertdev
Copy link
Contributor Author

Thanks for all the input, guys.
Since the aggregate-return check was removed from Travis, should I still go ahead and make the small changes for this PR?
Do you have a preference on returning small structs/pointers @b4n? Either way is fine with me, I'm just lazy.

@b4n
Copy link
Member

b4n commented Feb 25, 2017

@smostertdev For now I prefer not returning aggregates, but it's really your call as I can't really back my opinion up with facts right now, and @elextr and @codebrainz seem happy about it.

@codebrainz
Copy link
Member

@b4n I'm indifferent, I just mentioned that it's legal code.

@elextr
Copy link
Member

elextr commented Feb 25, 2017

@b4n, the more its discussed the more convinced I become that returns of small values via pointer parameters instead of the function return value is ugly and inefficient. It limits the benefits of inlining and defeats the compiler's ability to employ copy elision and requires the caller to explicitly declare a variable. GCC's aggregate-return warning should be changed to large-aggregate-return instead.

@frlan frlan added this to the 1.31.0 milestone Mar 5, 2017
Functions no longer work with structure, instead pointer to
structure.
Items are added with ui_add_document_sensitive so doc is
never null.
@frlan
Copy link
Member

frlan commented Apr 8, 2017

@codebrainz @b4n How going on here?

@frlan frlan removed this from the 1.31.0 milestone Jun 22, 2017
@frlan
Copy link
Member

frlan commented Jun 22, 2017

@b4n Please review as soon as you have some spare time. Will postpone it to another release

@frlan
Copy link
Member

frlan commented May 18, 2019

@b4n Ping ;)

@elextr
Copy link
Member

elextr commented May 18, 2019

@frlan, be patient, your previous ping isn't quite a year ago yet :)

@lpaulsen93
Copy link
Contributor

LGBI

@frlan frlan merged commit 9147b2b into geany:master Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants