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

Sync with latest upstream #527

Merged
merged 2 commits into from Feb 19, 2017
Merged

Sync with latest upstream #527

merged 2 commits into from Feb 19, 2017

Conversation

codebrainz
Copy link
Member

Closes #526

@@ -62,7 +62,7 @@ Editor_get_property(Editor *self, const gchar *prop_name)
PyObject *py_doc;
py_doc = (PyObject *) Document_create_new_from_geany_document(
self->editor->document);
if (py_doc && py_doc != Py_None)
if (!py_doc || py_doc == Py_None)
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't look into this difference, I just picked the kind of condition that made the branch execute when it seemed plausible to me.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, the previous one seems weird: returning None when py_doc is not NULL or None? weird.

New condition seems more reasonable to me. I wonder if Py_RETURN_NONE is different than return Py_None, but it doesn't change anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's effectively like

#define Py_RETURN_NONE do { Py_INCREF(Py_None); return Py_None; } while (0)

or something similar.

-DGEANYPY_PYTHON_DIR="\"$(libdir)/geany/geanypy\"" \
-DGEANYPY_PLUGIN_DIR="\"$(libdir)/geany\"" \
-DG_LOG_DOMAIN=\"GeanyPy\"
geanypy_la_CFLAGS = @GEANYPY_CFLAGS@ @GMODULE_CFLAGS@
geanypy_la_CFLAGS = @PYGTK_CFLAGS@ @GEANY_CFLAGS@ @GEANYPY_CFLAGS@ @GMODULE_CFLAGS@
Copy link
Member

Choose a reason for hiding this comment

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

I think the previous code is better. *_CFLAGS derived from PKG_CONFIG_CHECK() contain include paths (and usually nothing else) so they should be added to automake's *_CPPFLAGS indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really mind since they all end up jammed in the same part of the command line, but it seemed logical to group the pre-processor flags and the compiler flags into their respective variables.

Copy link
Member

Choose a reason for hiding this comment

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

*_CFLAGS derived from PKG_CONFIG_CHECK() contains mostly (usually exclusively) pre-processor flags, despite the name. And the order on the command line matters.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess as long as user environment CPPFLAGS override CPPFLAGS and likewise but after with CFLAGS it doesn't matter.

Edit: something like this

$(AM_CPPFLAGS) $(geanypy_la_CPPFLAGS) $(CPPFLAGS) $(AM_CFLAGS) $(geanypy_la_CFLAGS) $(CFLAGS)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this changes matters much, but IMO it's not bad, rather good. Yes, pkg-config will mostly fill the *_CFLAGS variable with preprocessor flags, but it might put other stuff here if it wants. Also, it's fine putting preprocessor flags in *_CFLAGS AFAIK, only library order matters.

@kugel-
Copy link
Member

kugel- commented Feb 17, 2017

One thing, the rest looks good to me.

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.

LGTM

-DGEANYPY_PYTHON_DIR="\"$(libdir)/geany/geanypy\"" \
-DGEANYPY_PLUGIN_DIR="\"$(libdir)/geany\"" \
-DG_LOG_DOMAIN=\"GeanyPy\"
geanypy_la_CFLAGS = @GEANYPY_CFLAGS@ @GMODULE_CFLAGS@
geanypy_la_CFLAGS = @PYGTK_CFLAGS@ @GEANY_CFLAGS@ @GEANYPY_CFLAGS@ @GMODULE_CFLAGS@
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this changes matters much, but IMO it's not bad, rather good. Yes, pkg-config will mostly fill the *_CFLAGS variable with preprocessor flags, but it might put other stuff here if it wants. Also, it's fine putting preprocessor flags in *_CFLAGS AFAIK, only library order matters.

@@ -62,7 +62,7 @@ Editor_get_property(Editor *self, const gchar *prop_name)
PyObject *py_doc;
py_doc = (PyObject *) Document_create_new_from_geany_document(
self->editor->document);
if (py_doc && py_doc != Py_None)
if (!py_doc || py_doc == Py_None)
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, the previous one seems weird: returning None when py_doc is not NULL or None? weird.

New condition seems more reasonable to me. I wonder if Py_RETURN_NONE is different than return Py_None, but it doesn't change anything.

@@ -0,0 +1,45 @@
#if defined(HAVE_CONFIG_H) && !defined(GEANYPY_WINDOWS)
Copy link
Member

Choose a reason for hiding this comment

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

no license header?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in each file.

@frlan frlan merged commit 59e7409 into geany:master Feb 19, 2017
@vfaronov
Copy link
Contributor

GeanyPy is listed in MAINTAINERS as “Currently diverged from upstream” — I figure that’s no longer correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants