Glynn Clements <glynn@gclements.plus.com> writes:
[...]
>> + vsprintf (buffer, template, ap);
> It would be nice to use vsnprintf(), but we would need to check that
> it's available (it's not in C89).
... And to use Gnulib-provided one if it isn't?
Ivan Shmakov wrote:
>> + vsprintf (buffer, template, ap);
> It would be nice to use vsnprintf(), but we would need to check that
> it's available (it's not in C89).... And to use Gnulib-provided one if it isn't?
I wouldn't bother adding an additional dependency.
Given the large size of the GRASS codebase and the relatively small
number of developers, pragmatism wins over perfectionism.
--
Glynn Clements <glynn@gclements.plus.com>
Glynn Clements <glynn@gclements.plus.com> writes:
>>>> + vsprintf (buffer, template, ap);
>>> It would be nice to use vsnprintf(), but we would need to check
>>> that it's available (it's not in C89).
>> ... And to use Gnulib-provided one if it isn't?
> I wouldn't bother adding an additional dependency.
But how on Earth could Gnulib be an additional dependency?
--cut: http://www.gnu.org/software/gnulib/--
Gnulib is a central location for common GNU code, intended to be
shared among GNU packages. GCC has libiberty, but this is hard to
disentangle from the GCC build tree. libit proved too hard to keep
up to date, and at this point is moribund.
Gnulib takes a different approach. Its components are intended to be
shared at the source level, rather than being a library that gets
built, installed, and linked against. Thus, there is no distribution
tarball; the idea is to copy files from Gnulib into your own source
tree.
--cut: http://www.gnu.org/software/gnulib/--
> Given the large size of the GRASS codebase and the relatively small
> number of developers, pragmatism wins over perfectionism.
Are they conflicting, or rather complementing each other in this
particular case?
Ivan Shmakov wrote:
>> Things I didn't change with this patch:
>> * buffers related to SQL commands, because I know no appropriate Cpp
>> macro for these;> I would suggest just adding e.g.:
> #define SQL_COMMAND_MAX 4000
> to the top of the file, then using that. Although it doesn't provide
> consistency between modules, it makes it easier to find such
> constants if they're at the top of the file.There're just too many files for this macro to be defined.
Could there be a separate header to define the macros like this
one?
Even if it's defined in a header, you still need to modify all those
files to use the constant in their array definitions.
AFAICT, most DB modules use the functions in lib/db/dbmi_base/string.c
(db_append_string etc) to construct SQL queries, rather than using
fixed-sized buffers.
[...]
>> * misused G_warning (), etc.; like:
[...]
>> there're just too many of these and I hope to handle them with a
>> script;> Note that those functions are tagged with:
> __attribute__((format(printf,1,2)));
> With gcc, you can use the option:
> `-Wformat-nonliteral' If `-Wformat' is specified, also warn if the
> format string is not a string literal and so cannot be checked,
> unless the format function takes its format arguments as a `va_list'.> to identify problematic cases (although --with-nls will interfere
> with this).It much more a problem to get them changed automatically than to
get them identified.
I don't think that an entirely automatic solution is desirable. Trying
to produce something which is robust enough not to introduce bugs in
some cases would probably take more time than making the changes
manually.
E.g. with Emacs, it's easy enough to perform a compilation, then
repeatedly use "C-x `" (next-error) and "C-x e" (call-last-kbd-macro)
to fix a hundred cases in a few minutes while maintaining manual
oversight of the process.
--
Glynn Clements <glynn@gclements.plus.com>
Glynn Clements <glynn@gclements.plus.com> writes:
>>>> Things I didn't change with this patch:
>>>> * buffers related to SQL commands, because I know no appropriate
>>>> Cpp macro for these;
>>> I would suggest just adding e.g.:
>>> #define SQL_COMMAND_MAX 4000
>>> to the top of the file, then using that.
[...]
>> There're just too many files for this macro to be defined. Could
>> there be a separate header to define the macros like this one?
> Even if it's defined in a header, you still need to modify all those
> files to use the constant in their array definitions.
Yes. Still, it saves a few lines (mostly context) per file in a
unified diff.
> AFAICT, most DB modules use the functions in
> lib/db/dbmi_base/string.c (db_append_string etc) to construct SQL
> queries, rather than using fixed-sized buffers.
Even if it's so, I've seen quite a few cases where it wasn't the
case.
>> [...]
>>>> * misused G_warning (), etc.; like:
>> [...]
>>>> there're just too many of these and I hope to handle them with a
>>>> script;
[...]
>> It much more a problem to get them changed automatically than to get
>> them identified.
> I don't think that an entirely automatic solution is desirable.
> Trying to produce something which is robust enough not to introduce
> bugs in some cases would probably take more time than making the
> changes manually.
Does changing the every occurence of (the whitespace was
inserted for the sake of readability):
^([[:blank:]]+) sprintf [[:blank:]]*
\(([[:alpha:]][[:alnum:]_]),
(("[^"]*") | _\(("[^"]*"))\)
[[:blank:]]* (,[[:blank:]]*[^)]+)?
\);
[[:blank:]]*
(G_(warning|fatal_error|([[:alpha:]_]*_)?message))
[[:blank:]]*
\(\2\);
with:
\1 \7 (_(\4\5), \6);
sound reasonable to you?
> E.g. with Emacs, it's easy enough to perform a compilation, then
> repeatedly use "C-x `" (next-error) and "C-x e" (call-last-kbd-macro)
> to fix a hundred cases in a few minutes while maintaining manual
> oversight of the process.
Indeed, I use C-x e quite extensively. But what's more of an
interest to me is the ability to describe the changes I'm
suggesting in a concise, yet machine-processable way. It's
hardly possible for the changes like the above with the unified
diff alone.
Ivan Shmakov wrote:
>> It much more a problem to get them changed automatically than to get
>> them identified.> I don't think that an entirely automatic solution is desirable.
> Trying to produce something which is robust enough not to introduce
> bugs in some cases would probably take more time than making the
> changes manually.Does changing the every occurence of (the whitespace was
inserted for the sake of readability):^([[:blank:]]+) sprintf [[:blank:]]*
\(([[:alpha:]][[:alnum:]_]),
(("[^"]*") | _\(("[^"]*"))\)
[[:blank:]]* (,[[:blank:]]*[^)]+)?
\);
[[:blank:]]*
(G_(warning|fatal_error|([[:alpha:]_]*_)?message))
[[:blank:]]*
\(\2\);with:
\1 \7 (_(\4\5), \6);
sound reasonable to you?
Nope. I'm not saying that there's anything wrong with regexp, just
that the amount of time required to understand it and predict the
consequences is likely to exceed the time taken for a manual solution.
For a start, there's no need to construct a regexp which accurately
matches all candidates. You can use -Wformat-nonliteral to get a
precise list of cases where the format string isn't a literal (which
will also eliminate false positives where the format string is a macro
which evaluates to a string literal). Also, bear in mind that most
G_fatal_error() format strings are encapsulated within the _() macro
for NLS; -Wformat-nonliteral will handle this, as it's looking at the
output from the pre-processor.
--
Glynn Clements <glynn@gclements.plus.com>
Ivan,
before your libgis patch gets lost: should it be applied?
According to Glynn it looked reasonable AFAIK.
Please send it to me (maybe offlist) for inclusion.
Markus
On Jan 14, 2008 1:06 AM, Glynn Clements <glynn@gclements.plus.com> wrote:
Ivan Shmakov wrote:
> > BTW, the correct pattern for issuing a string message is:
>
> > G_message ("%s", string);
>
> > not:
>
> > G_message (string);
>
> > [...]
>
> May I suggest the following patch?Seems reasonable enough.
> --- lib/gis/error.c 2008-01-03 14:15:58.000000000 +0600
> +++ lib/gis/error.c 2008-01-12 23:14:50.000000000 +0600
> @@ -93,6 +93,16 @@
> time_t, const char *);
> static int log_error (const char *, int);
>
> +static int
> +vfprint_error (int type, const char *template, va_list ap)
> +{
> + char buffer[2000]; /* G_asprintf does not work */
> +
> + vsprintf (buffer, template, ap);It would be nice to use vsnprintf(), but we would need to check that
it's available (it's not in C89).--
Glynn Clements <glynn@gclements.plus.com>_______________________________________________
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev
--
Open Source Geospatial Foundation
http://www.osgeo.org/
http://www.grassbook.org/
Markus Neteler <neteler@osgeo.org> writes:
> Ivan, before your libgis patch gets lost: should it be applied?
I believe it should.
> According to Glynn it looked reasonable AFAIK.
> Please send it to me (maybe offlist) for inclusion.
Surely.
BTW, since you apparently are using Google mail, you can try to
use `Download' on the ``Show original'' link for a message to
get it unaltered.
[...]
>>> + char buffer[2000]; /* G_asprintf does not work */
>>> +
>>> + vsprintf (buffer, template, ap);
>> It would be nice to use vsnprintf(), but we would need to check that
>> it's available (it's not in C89).
It would be even better to check for vasprintf () and use it if
it's available. Like:
static int
vfprint_error (int type, const char *template, va_list ap)
{
int result;
#ifdef HAVE_VASPRINTF
char *buffer;
vasprintf (&buffer, template, ap);
if (buffer == 0) {
/* . */
return -1;
}
result = print_error (buffer, type);
free (buffer);
#else
char buffer[2000]; /* G_asprintf does not work */
#ifdef HAVE_VSNPRINTF
vnsprintf (buffer, sizeof (buffer), template, ap);
#else
vsprintf (buffer, template, ap);
#endif
result = print_error (buffer, type);
#endif
/* . */
return result
}
However, I see no reason for why gnulib/lib/vasnprintf.c cannot
be used for a portable implementation of vasprintf (). Reusing
it would also obviate the need in lib/gis/asprintf.c. And,
well, doesn't eliminating extra code ease maintenance?
On Jan 26, 2008 7:12 PM, Ivan Shmakov <ivan@theory.asu.ru> wrote:
>>>>> Markus Neteler <neteler@osgeo.org> writes:
> Ivan, before your libgis patch gets lost: should it be applied?
I believe it should.
> According to Glynn it looked reasonable AFAIK.
> Please send it to me (maybe offlist) for inclusion.
Surely.
BTW, since you apparently are using Google mail, you can try to
use `Download' on the ``Show original'' link for a message to
get it unaltered.
Thanks, patch applied to SVN.
BTW: there is this (unrelated) warning:
error.c: In function 'print_sentence':
error.c:531: warning: value computed is not used
[...]
#ifdef HAVE_VASPRINTF
[...]
... no idea about this - sorry.
Markus
Markus Neteler <neteler@osgeo.org> writes:
[...]
> BTW: there is this (unrelated) warning: error.c: In function
> 'print_sentence': error.c:531: warning: value computed is not used
IIUC, it's harmless. The attached patch should make it go away.
[...]
On Jan 27, 2008 11:40 AM, Ivan Shmakov <ivan@theory.asu.ru> wrote:
[...]
> BTW: there is this (unrelated) warning: error.c: In function
> 'print_sentence': error.c:531: warning: value computed is not usedIIUC, it's harmless. The attached patch should make it go away.
Fixed in SVN. Thanks
Markus