-
Notifications
You must be signed in to change notification settings - Fork 272
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
Conversation
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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@ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
One thing, the rest looks good to me. |
There was a problem hiding this 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@ |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no license header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in each file.
GeanyPy is listed in |
Closes #526