diff mbox series

[1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid

Message ID 049989652cefb90304e711dbfe354b55a5a71f41.1603303474.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series fsmonitor inline / testing cleanup | expand

Commit Message

Alex Vandiver Oct. 21, 2020, 6:04 p.m. UTC 
From: Alex Vandiver <alexmv@dropbox.com>

These were inline'd when they were first introduced, presumably as an
optimization for cases when they were called in tight loops.  This
complicates using these functions, as untracked_cache_invalidate_path
is defined in dir.h.

Leave the inline'ing up to the compiler's decision, for ease of use.

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
---
 fsmonitor.c | 19 +++++++++++++++++++
 fsmonitor.h | 18 ++----------------
 2 files changed, 21 insertions(+), 16 deletions(-)

Comments

Taylor Blau Oct. 21, 2020, 8:55 p.m. UTC | #1 
On Wed, Oct 21, 2020 at 06:04:33PM +0000, Alex Vandiver via GitGitGadget wrote:
> From: Alex Vandiver <alexmv@dropbox.com>
>
> These were inline'd when they were first introduced, presumably as an
> optimization for cases when they were called in tight loops.  This
> complicates using these functions, as untracked_cache_invalidate_path
> is defined in dir.h.
>
> Leave the inline'ing up to the compiler's decision, for ease of use.

Letting the compiler inline these is fine, but...

> diff --git a/fsmonitor.h b/fsmonitor.h
> index 739318ab6d..6249020692 100644
> --- a/fsmonitor.h
> +++ b/fsmonitor.h
> @@ -49,14 +49,7 @@ void refresh_fsmonitor(struct index_state *istate);
>   * called any time the cache entry has been updated to reflect the
>   * current state of the file on disk.
>   */
> -static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)
> -{
> -	if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {
> -		istate->cache_changed = 1;
> -		ce->ce_flags |= CE_FSMONITOR_VALID;
> -		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name);
> -	}
> -}
> +extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce);
>
>  /*
>   * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate
> @@ -65,13 +58,6 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache
>   * trigger an lstat() or invalidate the untracked cache for the
>   * corresponding directory
>   */
> -static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)
> -{
> -	if (core_fsmonitor) {
> -		ce->ce_flags &= ~CE_FSMONITOR_VALID;
> -		untracked_cache_invalidate_path(istate, ce->name, 1);
> -		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
> -	}
> -}
> +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);
>
>  #endif

Any reason that these need to be externed explicitly? Note that these
functions are already externed by default since you haven't said
otherwise (and for no other reason than this'd be the only explicitly
externed function in fsmonitor.h).

Thanks,
Taylor
Junio C Hamano Oct. 21, 2020, 9:24 p.m. UTC | #2 
Taylor Blau <me@ttaylorr.com> writes:

>> +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);
> ...
> Any reason that these need to be externed explicitly? Note that these
> functions are already externed by default since you haven't said
> otherwise (and for no other reason than this'd be the only explicitly
> externed function in fsmonitor.h).

Possibly due to the recent discussion?

https://lore.kernel.org/git/xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com/
Taylor Blau Oct. 21, 2020, 9:31 p.m. UTC | #3 
On Wed, Oct 21, 2020 at 02:24:22PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > Any reason that these need to be externed explicitly? Note that these
> > functions are already externed by default since you haven't said
> > otherwise (and for no other reason than this'd be the only explicitly
> > externed function in fsmonitor.h).
>
> Possibly due to the recent discussion?
>
> https://lore.kernel.org/git/xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com/

Ah, thanks. I remember the thread, but I wasn't sure where the
discussion ended up. After re-reading it, it sounds like new function
declarations in header files should be prefixed with 'extern' (making
this patch correct as it already is).

Tangential to this discussion: are you still expecting a tree-wide
change to start use extern everywhere?

Thanks,
Taylor
Junio C Hamano Oct. 21, 2020, 9:38 p.m. UTC | #4 
Taylor Blau <me@ttaylorr.com> writes:

> Tangential to this discussion: are you still expecting a tree-wide
> change to start use extern everywhere?

I think before we start opening the tree for new topics is the best
time to do so, if we were to follow through, but after we have dealt
with brown-paper-bag fixes to the release.  The Makefile tweak for
the skip-dashed thing is the only one for 2.29, I think, so ...

Thanks.
Nipunn Koorapati Oct. 21, 2020, 11:22 p.m. UTC | #5 
> Letting the compiler inline these is fine, but...
>
> Any reason that these need to be externed explicitly? Note that these
> functions are already externed by default since you haven't said
> otherwise (and for no other reason than this'd be the only explicitly
> externed function in fsmonitor.h).

Did not have a reason or strong opinion here. It was this way, because this was
the way alexmv used it originally - but it does compile in either manner. The
thread Junio linked does seem to indicate preference for extern to avoid
confusion.

--Nipunn
diff mbox series

Patch

diff --git a/fsmonitor.c b/fsmonitor.c
index ca031c3abb..e120b3c5a9 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -287,6 +287,25 @@  void refresh_fsmonitor(struct index_state *istate)
 	istate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);
 }
 
+void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)
+{
+	if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {
+		istate->cache_changed = 1;
+		ce->ce_flags |= CE_FSMONITOR_VALID;
+		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name);
+	}
+}
+
+void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)
+{
+	if (core_fsmonitor) {
+		ce->ce_flags &= ~CE_FSMONITOR_VALID;
+		untracked_cache_invalidate_path(istate, ce->name, 1);
+		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
+	}
+}
+
+
 void add_fsmonitor(struct index_state *istate)
 {
 	unsigned int i;
diff --git a/fsmonitor.h b/fsmonitor.h
index 739318ab6d..6249020692 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -49,14 +49,7 @@  void refresh_fsmonitor(struct index_state *istate);
  * called any time the cache entry has been updated to reflect the
  * current state of the file on disk.
  */
-static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)
-{
-	if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {
-		istate->cache_changed = 1;
-		ce->ce_flags |= CE_FSMONITOR_VALID;
-		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name);
-	}
-}
+extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce);
 
 /*
  * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate
@@ -65,13 +58,6 @@  static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache
  * trigger an lstat() or invalidate the untracked cache for the
  * corresponding directory
  */
-static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)
-{
-	if (core_fsmonitor) {
-		ce->ce_flags &= ~CE_FSMONITOR_VALID;
-		untracked_cache_invalidate_path(istate, ce->name, 1);
-		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
-	}
-}
+extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);
 
 #endif