GNU bug report logs - #41242
Port feature/native-comp to Windows

Previous Next

Full log

Message #419 received at 41242 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
Subject: Re: bug#41242: Port feature/native-comp to Windows - Reduce the
 number of files probed when finding a lisp file.
Date: Mon, 01 Jun 2020 19:24:43 +0000

Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:

> Hi Andrea,
>
>> I could not compile this patch because non all the calls to openp has
>> been updated for the new parameter (my question on adding this stands).
>
> Sorry. I didn't check the GNU/Linux build.
>
>> In general please recall to check the stock build when working on
>> infrastructure integration, it's quite easy to break.
> I have tested that this new patch builds and bootstraps in windows x64,
> Ubuntu 18.04 amd64. Both with and without nativecomp.
>
>> Generally speaking I think the behavior we want to have is that when a
>> .eln file is specified this is loaded without re-adding
>> Vcomp_native_path_postfix.  I could not test it but I suspect this is
>> not handled.
>
> I tested moving company.eln to a directory in load-path without
> `comp-native-path-postfix` and then ran (load "company.eln") and it was
> loaded from that directory.
>
> Nico.

Hi Nico,

thanks this looks better.  I've two nits and two hopefully smarter
observations:

> +++ b/src/lread.c
> @@ -1056,31 +1056,25 @@ DEFUN ("get-load-suffixes", Fget_load_suffixes, Sget_load_suffixes, 0, 0, 0,
>      {
>        Lisp_Object exts = Vload_file_rep_suffixes;
>        Lisp_Object suffix = XCAR (suffixes);
> -      FOR_EACH_TAIL (exts)
> -	lst = Fcons (concat2 (suffix, XCAR (exts)), lst);
> -    }
> -  return Fnreverse (lst);
> -}
> +      bool native_code_suffix = (NATIVE_COMP_FLAG
> +        && strcmp (NATIVE_ELISP_SUFFIX, SSDATA (suffix)) == 0);

I think with the outer parentesys the second line should go be indented
at the level of NATIVE_COMP_FLAG.  I'd probably remove the parenthesys
and put the newline after the equal.

> -static Lisp_Object
> -effective_load_path (void)
> -{
> -#ifndef HAVE_NATIVE_COMP
> -  return Vload_path;

[...]

> +#ifdef HAVE_NATIVE_COMP
> +      CHECK_STRING_CAR (tail);
> +      char * suf = SSDATA (XCAR (tail));
> +      if (strcmp (NATIVE_ELISP_SUFFIX, suf) == 0)
> +        {
> +          CHECK_STRING (Vcomp_native_path_postfix);
> +          /* Here we add them in the opposite order so that nreverse
> +             corrects it.  */
> +          extended_suf = Fcons (Fcons (Qnil, XCAR (tail)), extended_suf);
> +          extended_suf = Fcons (Fcons (Vcomp_native_path_postfix, XCAR (tail)),
> +                                extended_suf);
> +        }
> +      else
> +#endif
> +        {
> +          extended_suf = Fcons (Fcons (Qnil, XCAR (tail)), extended_suf);
> +        }

I think GNU style does not want these brackets if the statement is just
one.

Okay interesting stuffs:

In which folders are we going to search if we do (load "...a/path/foo.eln")?

I believe in this case we should search the file only in "...a/path/"
because the user really want to load this specific file.  Am I correct?

That said IMO this logic is sufficiently complex to deserve a minimum of
testing to make sure we have it under control.  Not sure if the best
place is files-tests.el or comp-tests.el.

Maybe Eli likes to gives his opinion on this last point and on the patch
in general.

Thanks

  Andrea

--
akrl <at> sdf.org

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.