Quoting Oswald Buddenhagen (2023-08-22 15:51:20)
> On Tue, Aug 22, 2023 at 12:42:25PM +0200, Michiel van den Heuvel wrote:
> >> >I don't suppose you have some formatter config file?
> >> >
> >> bah! Real Hackers (TM) write their own formatters as shell-perl hybrids
> >> with recursive regular expressions. ON PUNCH CARDS.
> >
> >I'll trade you my magnetic needles so you can do it directly on disk
> >next time. ;)
> >
> :-)=)
>
> >Maybe just add it to the repository?
> >
> no, it feels "too off-topic".
> i'm attaching it for your masochistic pleasure. there you can see that
> it's actually not even aimed at isync specifically, because c++/qt?! i
> originally wrote this some 22 years ago, when astyle was Teh Shit.
> i wouldn't expect it to actually produce invariably correct results, and
> haven't used it on isync code for ... a very long time.
>
> >> >+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
> >> >+ va_start( va, fmt );
> >> >+ vfprintf( stderr, fmt, va );
> >> >
> >> totally on a tangent ...
> >> i have a bit of an aversion towards tearing apart format strings. it
> >> isn't a big deal here, because we're just printing to a stream with no
> >> concurrency, but if the target was syslog or a dynamically allocated
> >> string, things would look differently. this could be avoided by printing
> >> the "payload" to a temporary, but this would obviously have a runtime
> >> cost, esp. when using nfasprintf() to be super-safe. but as we already
> >> have xvprintf_core(), this overhead could be avoided by supporting
> >> recursive format strings: xprintf("%s:%d: %@\n", file, line, fmt, va).
> >> of course this would be total overkill here ... though once you have it,
> >> all kinds of use cases pop up. just thinking aloud ...
> >
> >Not fixed yet due to 1: the next point, and 2: do you want this?
> >
> depending on what you mean by "this".
>
> if you mean the "short" tangent of using a temporary: nope, not worth
> it.
>
> if you mean the "long" tangent of using xprintf: yes, but not in this
> patch series.
> for background, see the wip/better-stderr branch, which turns the
> personal aversion into an actual requirement. but notably, the config
> parser doesn't strictly _need_ that, because it runs before concurrency
> starts.
>
> >It would
> >introduce some duplication because conf_error and conf_sys_error print
> >to stderr and a buffer respectively. But if you see a way to avoid
> >this,
> >I'd like to hear it.
> >
> once the infra is there, strerror(errno) (and thus perror()) can be
> handled via the %m format (stolen from syslog), so things can be unified
> pretty much. the branch shows the direction.
>
> >> >+void
> >> >+conf_sys_error( conffile_t *cfile, const char *fmt, ... )
> >> >+{
> >> >+ va_list va;
> >> >+
> >> >+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
> >> >+ va_start( va, fmt );
> >> >+ vsys_error( fmt, va );
> >> >
> >> that call is theoretically unsafe; see the implementation.
> >
> >Sorry, my lack of experience with C is showing here. I looked at
> >vsys_error, but are not sure what you mean. Is it the `abort`? Do I fix
> >it with inlining `vsys_error` and a dynamically allocated buffer?
> >
> no, i mean the saving of errno - pedantically, you need to wrap the
> first fprintf().
> otoh, if the first print fails, the second one will most likely fail as
> well, and therefore it doesn't really matter if errno gets corrupted or
> not. but as i pushed e295f483 after much deliberation, i guess we should
> keep up the good practice.
>
> >Do you think that the `pclose` branch `ret < 0` error shouldn't be a
> >`conf_sys_error` but the normal one (`sys_error`)?
> >
> yeah, probably. and definitely it should match the popen one.
>
> > +static int
> > +read_cline( conffile_t *cfile )
> > +{
> > + if (cfile->include_fp) {
> > + cfile->include_line++;
> > + if ((cfile->rest = fgets( cfile->buf, cfile->bufl,
> > cfile->include_fp )))
>
> assigning cfile->rest right at the start would be less obscure. the
> pointer value is already known anyway.
That gets you an excess token error on (/after) the last line. It's not
about setting it to `buf`, but only doing it if there is actually a line
left to read.
> >@@ -325,9 +377,13 @@ getcline( conffile_t *cfile )
> >
> >+ if (cfile->cmd && !strcasecmp( cfile->cmd, "IncludeCmd" )) {
> >+ if (cfile->include_fp)
> >+ conf_error( cfile, "nested IncludeCmd\n" );
> >+ else
> >+ include_cmd_popen( cfile, cfile->val );
> >+ }
> >
> wouldn't just moving the body into the conditional below work?
> of course that would do the excess-argument check only after doing the
> popen, but that's consistent with how it works for other options.
> otoh, the side effects of instantly executing a potentially malformed
> command may be somewhat more grave than when processing other options.
> but handlign this properly would actually require a separate check
> anyway, as then we need to actually skip the command.
The problem with doing it after the popen is that `conf_print_loc` will then
indicate the
>
> >--- a/src/config.h
> >+++ b/src/config.h
> >@@ -12,10 +12,13 @@
> >
> > typedef struct {
> > const char *file;
> >+ char *include_command;
> >
> damn, i was clearly not clear enough. :}
> what i meant to say was that the "frontend" naming should be changed,
> while leaving the internal naming (of variables and functions) at
> "eval", because it's shorter (and arguably clearer without the wider
> context of the options in the other file).
>
> regards_______________________________________________
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel
