Hi Junio, > Le 2 sept. 2020 à 13:21, Junio C Hamano <gitster@xxxxxxxxx> a écrit : > > "Philippe Blain via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> diff --git a/.gitignore b/.gitignore >> index ee509a2ad2..f4c51300e0 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -197,6 +197,7 @@ >> /git.spec >> *.exe >> *.[aos] >> +*.o.json > > Does this naming/suffix follow the common practice followed among > those projects that use the -MJ option? Even though I may have > heard of it, I am not familiar with the use of the feature at all, > and to such a person, naming a file after what format it is written > in (i.e. 'json') feels a lot less useful than what it contains > (i.e. compilation database entries). > > It's like naming our source files with .txt suffix ;-) This addition to the .gitignore is for the individual JSON files (one per source file), that are placed in the $(compdb_dir). I think naming "rebase.o.json" the JSON file that describes how to compile "rebase.c" into "rebase.o" makes sense. I don't know what is the convention for other projects. >> +# Define GENERATE_COMPILATION_DATABASE to generate JSON compilation database >> +# entries during compilation if your compiler supports it, using the `-MJ` flag. >> +# The JSON entries will be placed in the `compile_commands/` directory, >> +# and the JSON compilation database 'compile_commands.json' will be created >> +# at the root of the repository. > > Likewise for the name of the directory and the resulting single > database file. I just want to make sure that we are following the > common convention, so people familiar with the use of the feature > would feel at home, so a simple answer "yes" would suffice. The name `compile_commands.json` for the database itself is standard. The name of the directory where the '*.o.json' files are placed is a name I chose, and I don't feel strongly about it. I thought it made sense to name it like that, then its purpose is clear. We could make it a hidden directory if we don't want to add a new folder to the root of the repo when using this feature. >> +ifdef GENERATE_COMPILATION_DATABASE >> +compdb_check = $(shell $(CC) $(ALL_CFLAGS) \ >> + -c -MJ /dev/null \ >> + -x c /dev/null -o /dev/null 2>&1; \ >> + echo $$?) >> +ifeq ($(compdb_check),0) >> +override GENERATE_COMPILATION_DATABASE = yes > > This feels strange. If the end user said to GENERATE and we find we > are capable, we still override to 'yes'? What if the end user set > 'no' to the GENERATE_COMPILATION_DATABASE macro? Shouldn't we be > honoring that wish? We should. I'll tweak (and simplify) that for v3. >> @@ -2381,16 +2401,30 @@ missing_dep_dirs = >> dep_args = >> endif >> >> +compdb_dir = compile_commands/ >> + >> +ifeq ($(GENERATE_COMPILATION_DATABASE),yes) >> +missing_compdb_dir = $(compdb_dir) >> +$(missing_compdb_dir): >> + @mkdir -p $@ >> + >> +compdb_file = $(compdb_dir)$(subst .-,,$(subst /,-,$(dir $@)))$(notdir $@).json > > This detail does not matter as long as the end result ensures unique > output for all source files, but I am having trouble guessing what > the outermost subst, which removes ".-" sequence, is trying to make > prettier. Care to explain? Yes, it is because the `$(dir $@)` Makefile function will return `./` for source files at the base of the repo, so the JSON files get named eg. `.-rebase.o.json` and then they are hidden. So it's just to make them non-hidden, so as not to confuse someone that would count the number of source files and compare with the number of (non-hidden) '*.o.json' files in $(comdb_dir) and get a different number. >> @@ -2413,6 +2447,14 @@ else >> $(OBJECTS): $(LIB_H) $(GENERATED_H) >> endif >> >> +ifeq ($(GENERATE_COMPILATION_DATABASE),yes) >> +all:: compile_commands.json >> +compile_commands.json: >> + @$(RM) $@ >> + $(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)*.o.json > $@+ > > OK. The entire thing is concatenated and enclosed by a single pair > of '[' and ']'. Yes, and the comma after the last entry removed for JSON compliance. > If we are planning to allow developers customize compdb_dir, > requiring trailing slash in the value of $(compdb_dir) is not very > friendly. OK I'll change that. > Thanks. Thank you for your comments! Philippe.
