Refactoring

[fpottier]

This PR introduces the functions expand_ident and expand_macro_application so as to clarify how identifiers are treated by cppo. In particular, in the case where an identifier is not a macro, the code is significantly simpler than before; it is now clear that such an identifier is treated as text.

There should be no change in functionality.

I wanted to propose this simplification before introducing support for #scope ... #endscope.

[pmetzger]

What testing did you do to make sure the functionality remained the same?

[fpottier]

I just verified that make test still succeeds.

If you think that the current test suite is too small, I would be happy to strengthen it. We could for instance copy the source code of some packages that have a heavy dependency on cppo and integrate it in the test suite?

[fpottier]

Or perhaps I could create a script that performs a more heavyweight test, such as creating a fresh opam switch, installing cppo, and compiling a bunch of packages that depend (directly) on cppo. (This would allow checking that these packages can be still compiled, but would not allow us to detect a situation where the output of cppo changes slightly.)

[liyishuai]

Or perhaps I could create a script that performs a more heavyweight test, such as creating a fresh opam switch, installing cppo, and compiling a bunch of packages that depend (directly) on cppo. (This would allow checking that these packages can be still compiled, but would not allow us to detect a situation where the output of cppo changes slightly.)

I think that's part of the CI (which is passing):

cppo/.github/workflows/build.yml

Lines 176 to 198 in 21f0586

	- name: Test dependants
	if: >
	(matrix.ocaml-version >= '4.05') && (matrix.os != 'windows-latest')
	run: |
	PACKAGES=`opam list -s --color=never --installable --depends-on cppo,cppo_ocamlbuild`
	echo "Dependants:" $PACKAGES
	for PACKAGE in $PACKAGES
	do
	echo $SKIP_BUILD | tr ' ' '\n' | grep ^$PACKAGE$ > /dev/null &&
	echo Skip $PACKAGE && continue
	OPAMWITHTEST=true
	echo $SKIP_TEST | tr ' ' '\n' | grep ^$PACKAGE$ > /dev/null &&
	OPAMWITHTEST=false
	([ $OPAMWITHTEST == false ] &&
	echo ::group::Build $PACKAGE) ||
	echo ::group::Build and test $PACKAGE
	DEPS_FAILED=false
	(opam depext $PACKAGE &&
	opam install --deps-only $PACKAGE) || DEPS_FAILED=true
	[ $DEPS_FAILED == false ] && opam install $PACKAGE
	echo ::endgroup::
	[ $DEPS_FAILED == false ] || echo Dependencies broken
	done

[fpottier]

I think that's part of the CI

Thanks, I didn't know.

[liyishuai]

Given that all downstream dependants still compile, propose merging if no other concerns arise by Fri 13 Sep AoE.

[pmetzger]

I do think we should improve the test suite, fwiw.

[liyishuai]

I do think we should improve the test suite, fwiw.

• Tracking in Test suite improvement #95