All of lore.kernel.org
 help / color / mirror / Atom feed
From: Emily Shaffer <nasamuffin@google.com>
To: Git List <git@vger.kernel.org>
Cc: chooglen@google.com, newren@gmail.com, cscallsign@gmail.com
Subject: How to discourage introduction of new globals? (was: Video conference libification eng discussion - notes from today)
Date: Thu, 20 Apr 2023 10:08:30 -0700	[thread overview]
Message-ID: <ZEFxjmt9zV6V384b@google.com> (raw)
In-Reply-To: <CAJoAoZnoz9rsEEUGoG4BKMwW7r_Q-H2JcD_SVxuA_ykDZ33J8w@mail.gmail.com>

On Thu, Apr 20, 2023 at 10:01:45AM -0700, Emily Shaffer wrote:
> *   How can we avoid introducing new globals?
>     *   Elijah: for example, diffcore added a new global for actually no reason
>     *   Getting rid of the\_repository is so daunting
>     *   Emily: should we introduce some CI rule around new globals in
> the diff? Update SubmittingPatches?
>     *   Glen: tests which enforce libification also make it difficult
> to introduce new globals in libraries. But in some places, like
> setting something across the entire process lifetime, we don't have a
> better pattern to replace it with, right?
>     *   Elijah: in the short term, we do have dozens of globals;
> should they go in one place (so we know where to get rid of them)?
> Should they go with the file that is using them the most, or
> something?
>     *   Emily: does putting all the globals in one place make it
> easier to tell whether a given path is using a certain global?
>     *   Cem: Putting it in one file makes it really easy to tell
> people they shouldn't introduce a new global! E.g. adding a comment at
> the top of `globals.h` like /\* are you sure you need to add a global
> here? \*/
>     *   Emily: can coccinelle help?
>         *   Glen: probably not, coccinelle runs over all files whether
> they're unchanged or not
>     *   Emily: does wrapping globals in a getter that traces help?
>         *   Elijah: but people will just bypass the getter, unless you
> restructure the code so the accessors are the only thing that are
> accessible
>         *   Cem: This is basically a global to singleton conversion -
> but there's no way to guarantee that new globals follow that pattern
> either
>     *   Emily: I'll spin this out into a separate thread after this meeting

Any thoughts from the rest of the project? To summarize goals:

- The path to library code stays about as hard as it is, doesn't get
  harder by the addition of more globals.
- It's very easy to tell when globals are being used in order to make
  cleaning them up less painful.
- The onus for avoiding new globals doesn't fall entirely on a handful
  of reviewers, who may miss something newly added.

      reply	other threads:[~2023-04-20 17:08 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-20 17:01 Video conference libification eng discussion - notes from today Emily Shaffer
2023-04-20 17:08 ` Emily Shaffer [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZEFxjmt9zV6V384b@google.com \
    --to=nasamuffin@google.com \
    --cc=chooglen@google.com \
    --cc=cscallsign@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.