Hello Albert, Am 03.03.2018 um 23:26 schrieb Albert Astals Cid: > El dissabte, 3 de març de 2018, a les 21:15:48 CET, Adam Reichold va escriure: >> And this time around, without a bunch of typos... Sorry for the noise. > > Wouldn't it make more sense to have the void_t definition in the anonymous > namespace in gfile.cc for now until we really need to use it somewhere else?
I am unsure if the next user will find it if it is tucked away there, but attached is the simplified patch the does this and hence only touches gfile.cc. Best regards, Adam. > Cheers, > Albert > >> >> Am 03.03.2018 um 21:03 schrieb Adam Reichold: >>> Hello again, >>> >>> attached is a patch against master that fixes the Clang/libc++ linking >>> issue with the cpp frontend's text_box_data destructor and the >>> non-standard high-resolution mtime field name on Mac OS X. >>> >>> Best regards, Adam. >>> >>> Am 03.03.2018 um 20:18 schrieb Albert Astals Cid: >>>> El divendres, 2 de març de 2018, a les 13:24:37 CET, suzuki toshiya va >>>> >>>> escriure: >>>>> Dear Adam, >>>>> >>>>> Thank you always for rewriting in C++ way! >>>>> >>>>>> If you are uncomfortable with the CMake- and preprocessor-based >>>>>> solution, you can solve such issues using templates as shown in >>>>>> >>>>>> https://github.com/adamreichold/poppler/commit/68f46dce62ad97bdbb22bf79 >>>>>> ac5 >>>>>> 0c128d899a302 >>>>> >>>>> Waao, it looks very smart. I'm quite sure that such smart idea >>>>> cannot come to my rusted head living in C89 world :-) >>>>> I'm *not* uncomfortable with cmake + preprocessor solution, but >>>>> yours is far better than mine. >>>>> >>>>> If somebody finds new variant using something different from >>>>> st_mtim, st_mtimespec - in my patch, CMakeList.txt & gfiles.cc >>>>> should be changed, and re-run cmake. But in your patch, only >>>>> gfiles.cc is needed to be modified, and no need to re-run cmake. >>>>> It would be easier for further tweaking (if somebody needs). >>>>> >>>>> I want to hear other reviewers comment. >>>> >>>> This kind of template substitution by "failure" are a bit weird to get >>>> around, but on the other hand it's quite self contained and i guess you >>>> could add some text explaining "this substitution is used on Darwin" and >>>> "this substitution is used on Linux". >>>> >>>> I'm fine with that, but you're going to need to send a patch against >>>> poppler git not against mpsuzuki's repo if you want inclusion upstream >>>> :) >>>> >>>> Cheers, >>>> >>>> Albert >>>>> >>>>> Regards, >>>>> mpsuzuki >>>>> >>>>> Adam Reichold wrote: >>>>>> Hello, >>>>>> >>>>>> If it works on POSIX and builds on Darwin, it looks good to me. What I >>>>>> would like would be else clauses in the CMake and preprocessor >>>>>> definitions that give proper error messages. (Or maybe use the POSIX >>>>>> variant as the default and only use mtimespec if as an override.) >>>>>> >>>>>> If you are uncomfortable with the CMake- and preprocessor-based >>>>>> solution, you can solve such issues using templates as shown in >>>>>> >>>>>> https://github.com/adamreichold/poppler/commit/68f46dce62ad97bdbb22bf79 >>>>>> ac5 >>>>>> 0c128d899a302 >>>>>> >>>>>> with the related Travis build being >>>>>> >>>>>> https://travis-ci.org/adamreichold/poppler/builds/348193138 >>>>>> >>>>>> Best regards, Adam. >>>>>> >>>>>> Am 02.03.2018 um 03:34 schrieb suzuki toshiya: >>>>>>> Hi, >>>>>>> >>>>>>> It seems that the counterpart in macOS libc corresponding to >>>>>>> stat.st_mtim is stat.st_mtimespec. >>>>>>> https://opensource.apple.com/source/xnu/xnu-201.5/bsd/sys/stat.h.auto. >>>>>>> htm >>>>>>> l >>>>>>> >>>>>>> I wrote a patch testing st_mtim availability by >>>>>>> CHECK_STRUCT_HAS_MEMBER() >>>>>>> suggested by William, and also testing st_mtimespec too, and reflect >>>>>>> the result to the macro GET_MTIM_FROM_STATBUF(). >>>>>>> https://github.com/mpsuzuki/poppler/commit/79d00ac08d672d572a7ec310b5a >>>>>>> 27e >>>>>>> b66c956e4c >>>>>>> >>>>>>> Building on travis-ci.org finishes successfully. Yet I'm >>>>>>> unsure such macro is following to the coding style of poppler. >>>>>>> Also if anybody has a testing code to evaluate the code works >>>>>>> well (do you have to make 2 file with nsec difference of the >>>>>>> timestamp?). Please give me comment... >>>>>>> >>>>>>> Regards, >>>>>>> mpsuzuki >>>>>>> >>>>>>> On 2/19/2018 1:42 PM, William Bader wrote: >>>>>>>> Can you test for it in cmake? >>>>>>>> https://cmake.org/cmake/help/v3.0/module/CheckStructHasMember.html >>>>>>>> >>>>>>>> ________________________________ >>>>>>>> From: poppler <poppler-boun...@lists.freedesktop.org> on behalf of >>>>>>>> Jeroen Ooms <jer...@berkeley.edu> Sent: Sunday, February 18, 2018 >>>>>>>> 6:29 >>>>>>>> PM >>>>>>>> To: Ihar Filipau >>>>>>>> Cc: poppler@lists.freedesktop.org >>>>>>>> Subject: Re: [poppler] gfile.cc fails to build on macos due to >>>>>>>> statbuf.st_mtim>>> >>>>>>>> On Mon, Feb 12, 2018 at 3:04 PM, Ihar Filipau <thephil...@gmail.com> >>>> >>>> wrote: >>>>>>>>> On 2/12/18, Jeroen Ooms <jer...@berkeley.edu> wrote: >>>>>>>>>> On Sun, Feb 11, 2018 at 12:11 PM, Albert Astals Cid <aa...@kde.org> >>>> >>>> wrote: >>>>>>>>>>> You're never assigning to tv_nsec in there but still use it in a >>>>>>>>>>> comparison, >>>>>>>>>>> that needs fixing. >>>>>>>>>> >>>>>>>>>> You are right. I think we should compare modification time only by >>>>>>>>>> seconds. The standard definition of 'struct stat' only specifies >>>>>>>>>> st_ctime, so I don't think there is a portable way to get >>>>>>>>>> nanoseconds: >>>>>>>>>> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h. >>>>>>>>>> htm >>>>>>>>>> l >>>>>>>>> >>>>>>>>> That's an old version of POSIX. Check the newer version: >>>>>>>>> >>>>>>>>> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h. >>>>>>>>> htm >>>>>>>>> l >>>>>>>>> IOW, there is a standard portable way - since 2008, 10 years ago. >>>>>>>>> It's >>>>>>>>> just Mac OS X hasn't updated its POSIX support after v6, from >>>>>>>>> 2004. >>>>>>>> >>>>>>>> OK so how do you suggest this should be fixed? It would be great if >>>>>>>> things would keep working on Mac OS. >>>>>>> >>>>>>> _______________________________________________ >>>>>>> poppler mailing list >>>>>>> poppler@lists.freedesktop.org >>>>>>> https://lists.freedesktop.org/mailman/listinfo/poppler >>>>> >>>>> _______________________________________________ >>>>> poppler mailing list >>>>> poppler@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/poppler >>>> >>>> _______________________________________________ >>>> poppler mailing list >>>> poppler@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/poppler > > > > >
From 32d8a5a43acb9c655215a1fcd10a09c4c619264d Mon Sep 17 00:00:00 2001
From: Adam Reichold <adam.reich...@t-online.de>
Date: Mon, 12 Feb 2018 08:09:00 +0100
Subject: [PATCH 1/2] Explicitly anchor destructor of text_box_data to avoid
linker errors using Clang on Mac OS X.
---
cpp/poppler-page.cpp | 2 ++
cpp/poppler-private.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/cpp/poppler-page.cpp b/cpp/poppler-page.cpp
index 83d48f07..9ae569b0 100644
--- a/cpp/poppler-page.cpp
+++ b/cpp/poppler-page.cpp
@@ -290,6 +290,8 @@ ustring page::text(const rectf &r, text_layout_enum layout_mode) const
/*
* text_box object for page::text_list()
*/
+text_box_data::~text_box_data() = default;
+
text_box::~text_box() = default;
text_box::text_box(text_box_data *data) : m_data{data}
diff --git a/cpp/poppler-private.h b/cpp/poppler-private.h
index 3753567f..c4ca6e57 100644
--- a/cpp/poppler-private.h
+++ b/cpp/poppler-private.h
@@ -70,6 +70,8 @@ void delete_all(const Collection &c)
struct text_box_data
{
+ ~text_box_data();
+
ustring text;
rectf bbox;
std::vector<rectf> char_bboxes;
--
2.16.2
From 0e5489d08a1fe4409d6abf9c9c67862ee3ae2931 Mon Sep 17 00:00:00 2001
From: Adam Reichold <adam.reich...@t-online.de>
Date: Sun, 4 Mar 2018 09:17:00 +0100
Subject: [PATCH 2/2] Use the detection idiom to handle the non-standard struct
stat field name for high-resolution mtime on Mac OS X.
---
goo/gfile.cc | 38 ++++++++++++++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/goo/gfile.cc b/goo/gfile.cc
index e4c9b9fb..7fe7dd89 100644
--- a/goo/gfile.cc
+++ b/goo/gfile.cc
@@ -65,6 +65,40 @@
#define PATH_MAX 1024
#endif
+namespace {
+
+template< typename... >
+struct void_type
+{
+ using type = void;
+};
+
+template< typename... Args >
+using void_t = typename void_type< Args... >::type;
+
+template< typename Stat, typename = void_t<> >
+struct StatMtim
+{
+ static const struct timespec& value(const Stat& stbuf) {
+ return stbuf.st_mtim;
+ }
+};
+
+// Mac OS X uses a different field name than POSIX and this detects it.
+template< typename Stat >
+struct StatMtim< Stat, void_t< decltype ( Stat::st_mtimespec ) > >
+{
+ static const struct timespec& value(const Stat& stbuf) {
+ return stbuf.st_mtimespec;
+ }
+};
+
+inline const struct timespec& mtim(const struct stat& stbuf) {
+ return StatMtim< struct stat >::value(stbuf);
+}
+
+}
+
//------------------------------------------------------------------------
GooString *getCurrentDir() {
@@ -687,7 +721,7 @@ GooFile::GooFile(int fdA)
{
struct stat statbuf;
fstat(fd, &statbuf);
- modifiedTimeOnOpen = statbuf.st_mtim;
+ modifiedTimeOnOpen = mtim(statbuf);
}
bool GooFile::modificationTimeChangedSinceOpen() const
@@ -695,7 +729,7 @@ bool GooFile::modificationTimeChangedSinceOpen() const
struct stat statbuf;
fstat(fd, &statbuf);
- return modifiedTimeOnOpen.tv_sec != statbuf.st_mtim.tv_sec || modifiedTimeOnOpen.tv_nsec != statbuf.st_mtim.tv_nsec;
+ return modifiedTimeOnOpen.tv_sec != mtim(statbuf).tv_sec || modifiedTimeOnOpen.tv_nsec != mtim(statbuf).tv_nsec;
}
#endif // _WIN32
--
2.16.2
signature.asc
Description: OpenPGP digital signature
_______________________________________________ poppler mailing list poppler@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/poppler
