Signed-off-by: Barbu Paul - Gheorghe barbu.paul.gheorghe@gmail.com --- lib/libalpm/alpm_list.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/alpm_list.c b/lib/libalpm/alpm_list.c index 39eded1..b9e7cba 100644 --- a/lib/libalpm/alpm_list.c +++ b/lib/libalpm/alpm_list.c @@ -21,13 +21,11 @@ #include <stdlib.h> #include <string.h> +#include "util.h" + /* libalpm */ #include "alpm_list.h" -/* check exported library symbols with: nm -C -D <lib> */ -#define SYMEXPORT __attribute__((visibility("default"))) -#define SYMHIDDEN __attribute__((visibility("internal"))) - /** * @addtogroup alpm_list List Functions * @brief Functions to manipulate alpm_list_t lists.
Sorry for the mess with the empty attachment, I still have to learn how to properly use git send-email and git imap-send, this is the first patch I ever submitted this way.
Have a nice day.
On Thu, Jul 26, 2012 at 3:23 PM, Paul Barbu Gheorghe barbu.paul.gheorghe@gmail.com wrote:
Signed-off-by: Barbu Paul - Gheorghe barbu.paul.gheorghe@gmail.com
lib/libalpm/alpm_list.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/alpm_list.c b/lib/libalpm/alpm_list.c index 39eded1..b9e7cba 100644 --- a/lib/libalpm/alpm_list.c +++ b/lib/libalpm/alpm_list.c @@ -21,13 +21,11 @@ #include <stdlib.h> #include <string.h> +#include "util.h"
/* libalpm */ #include "alpm_list.h" -/* check exported library symbols with: nm -C -D <lib> */ -#define SYMEXPORT __attribute__((visibility("default"))) -#define SYMHIDDEN __attribute__((visibility("internal")))
The reasoning behind this is alpm_list.c/h is a completely standalone set of files, so we don't link back to anything in the rest of alpm. With that said, the duplicates were quite on purpose and I'm not inclined to apply this patch.
-Dan
On Thu, Jul 26, 2012 at 03:51:20PM -0500, Dan McGee wrote:
On Thu, Jul 26, 2012 at 3:23 PM, Paul Barbu Gheorghe barbu.paul.gheorghe@gmail.com wrote:
Signed-off-by: Barbu Paul - Gheorghe barbu.paul.gheorghe@gmail.com
lib/libalpm/alpm_list.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/alpm_list.c b/lib/libalpm/alpm_list.c index 39eded1..b9e7cba 100644 --- a/lib/libalpm/alpm_list.c +++ b/lib/libalpm/alpm_list.c @@ -21,13 +21,11 @@ #include <stdlib.h> #include <string.h> +#include "util.h"
/* libalpm */ #include "alpm_list.h" -/* check exported library symbols with: nm -C -D <lib> */ -#define SYMEXPORT __attribute__((visibility("default"))) -#define SYMHIDDEN __attribute__((visibility("internal")))
The reasoning behind this is alpm_list.c/h is a completely standalone set of files, so we don't link back to anything in the rest of alpm. With that said, the duplicates were quite on purpose and I'm not inclined to apply this patch.
-Dan
Hmmm, so this is totally non-obvious looking at the code and at history. Why do we care about keeping alpm_list as an island? It's not treated any differently in the buildsystem, and both alpm.h and alpm_list.h expose symbols in a singular shared object.
If we aren't going to merge this, we should at least document the duplication.
d
On 27/07/12 06:59, Dave Reisner wrote:
On Thu, Jul 26, 2012 at 03:51:20PM -0500, Dan McGee wrote:
On Thu, Jul 26, 2012 at 3:23 PM, Paul Barbu Gheorghe barbu.paul.gheorghe@gmail.com wrote:
Signed-off-by: Barbu Paul - Gheorghe barbu.paul.gheorghe@gmail.com
lib/libalpm/alpm_list.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/alpm_list.c b/lib/libalpm/alpm_list.c index 39eded1..b9e7cba 100644 --- a/lib/libalpm/alpm_list.c +++ b/lib/libalpm/alpm_list.c @@ -21,13 +21,11 @@ #include <stdlib.h> #include <string.h> +#include "util.h"
/* libalpm */ #include "alpm_list.h" -/* check exported library symbols with: nm -C -D <lib> */ -#define SYMEXPORT __attribute__((visibility("default"))) -#define SYMHIDDEN __attribute__((visibility("internal")))
The reasoning behind this is alpm_list.c/h is a completely standalone set of files, so we don't link back to anything in the rest of alpm. With that said, the duplicates were quite on purpose and I'm not inclined to apply this patch.
-Dan
Hmmm, so this is totally non-obvious looking at the code and at history. Why do we care about keeping alpm_list as an island? It's not treated any differently in the buildsystem, and both alpm.h and alpm_list.h expose symbols in a singular shared object.
If we aren't going to merge this, we should at least document the duplication.
I believe the idea was to be able to just use alpm_list.{h,c} in another project without linking to libalpm.
Personally, I see no need to keep that separation. There are more general list implementations widely available and I have never seen another project use this.
Allan
On 07/31/2012 02:12 AM, Allan McRae wrote:
I believe the idea was to be able to just use alpm_list.{h,c} in another project without linking to libalpm.
Personally, I see no need to keep that separation. There are more general list implementations widely available and I have never seen another project use this.
OK, I get it.
So it's up to you guys if you merge it, just let me know when you take a decision.
On 07/26/2012 11:51 PM, Dan McGee wrote:
The reasoning behind this is alpm_list.c/h is a completely standalone set of files, so we don't link back to anything in the rest of alpm.
Why this decision?
I mean, there should be a good reson in order to duplicate code...
I'm trying to understand the things here, don't get me wrong.
On 27/07/12 06:51, Dan McGee wrote:
On Thu, Jul 26, 2012 at 3:23 PM, Paul Barbu Gheorghe barbu.paul.gheorghe@gmail.com wrote:
Signed-off-by: Barbu Paul - Gheorghe barbu.paul.gheorghe@gmail.com
lib/libalpm/alpm_list.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/alpm_list.c b/lib/libalpm/alpm_list.c index 39eded1..b9e7cba 100644 --- a/lib/libalpm/alpm_list.c +++ b/lib/libalpm/alpm_list.c @@ -21,13 +21,11 @@ #include <stdlib.h> #include <string.h> +#include "util.h"
/* libalpm */ #include "alpm_list.h" -/* check exported library symbols with: nm -C -D <lib> */ -#define SYMEXPORT __attribute__((visibility("default"))) -#define SYMHIDDEN __attribute__((visibility("internal")))
The reasoning behind this is alpm_list.c/h is a completely standalone set of files, so we don't link back to anything in the rest of alpm. With that said, the duplicates were quite on purpose and I'm not inclined to apply this patch.
Cleaning out patch backlog...
Our choices are: 1) accept patch 2) add comment in alpm_list.c that it is a stand alone file
Personally, I doubt anyone is taking alpm_list.{h,c} and using it anywhere. There are plenty of more widely used list implementations out there. So I do not care which solution is used.
@Dan: I'm going to let you have final say here. Just a yes/no needed.
Thanks, Allan
On Tue, May 28, 2013 at 11:43 PM, Allan McRae allan@archlinux.org wrote:
On 27/07/12 06:51, Dan McGee wrote:
On Thu, Jul 26, 2012 at 3:23 PM, Paul Barbu Gheorghe barbu.paul.gheorghe@gmail.com wrote:
Signed-off-by: Barbu Paul - Gheorghe barbu.paul.gheorghe@gmail.com
lib/libalpm/alpm_list.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/alpm_list.c b/lib/libalpm/alpm_list.c index 39eded1..b9e7cba 100644 --- a/lib/libalpm/alpm_list.c +++ b/lib/libalpm/alpm_list.c @@ -21,13 +21,11 @@ #include <stdlib.h> #include <string.h> +#include "util.h"
/* libalpm */ #include "alpm_list.h" -/* check exported library symbols with: nm -C -D <lib> */ -#define SYMEXPORT __attribute__((visibility("default"))) -#define SYMHIDDEN __attribute__((visibility("internal")))
The reasoning behind this is alpm_list.c/h is a completely standalone set of files, so we don't link back to anything in the rest of alpm. With that said, the duplicates were quite on purpose and I'm not inclined to apply this patch.
Cleaning out patch backlog...
Our choices are:
- accept patch
- add comment in alpm_list.c that it is a stand alone file
Personally, I doubt anyone is taking alpm_list.{h,c} and using it anywhere. There are plenty of more widely used list implementations out there. So I do not care which solution is used.
@Dan: I'm going to let you have final say here. Just a yes/no needed.
I vote #2.
And then switch to glib [1]. :)
-Dan
[1] https://developer.gnome.org/glib/2.30/glib-Pointer-Arrays.html
Did anyone get 5 mins. to write an answer for my question?
Why isn't desirable to link alpm_list.{c,h} back to the rest of alpm?
pacman-dev@lists.archlinux.org