Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Comments can be explicit, but without compromising readability direct. #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TainoT
Copy link

@TainoT TainoT commented Apr 10, 2013

No description provided.

@maugier
Copy link
Contributor

maugier commented Apr 10, 2013

The systematic variable renaming goes against the direction the maintainer has consistently taken before. It is also likely to break yet unmerged code.

The new names are also much less readable - they do not indicate that they refer to variables. This is likely to cause huge problems with future versions of Make which may introduce single-assignment variables.

On the other hand, the hash comments look quite good -- I am in favor of including these changes if you cherry-pick them and submit them as separate commits. Otherwise I advise the maintainer against accepting the pull request.

Totally serious here,
--Max

@ayekat
Copy link

ayekat commented Nov 24, 2017

I think some of the variables are actually not variables, but constants. For example VARIABLECONTAININGTHEFILEEXTENSIONFORHTMLFILE should really be CONSTANTCONTAININGTHEFILEEXTENSIONFORHTMLFILE. The naming is currently misleading and should be fixed.

@maugier
Copy link
Contributor

maugier commented Nov 24, 2017

GNU Make 4.1, which we can consider the reference implementation of the Makefile language, currently does not distinguish between variable and constants, but future alternate implementations may, as part of various extensions to the language.

Therefore, it is important to distinguish these (constant) variables from possible future constants, which may be implemented with different syntax and semantics.

VARIABLECONTAININGTHEFILEEXTENSIONFORHTMLFILE should thus be renamed, not to CONSTANTCONTAININGTHEFILEEXTENSIONFORHTMLFILE, but to CONSTANTVARIABLECONTAININGTHEFILEEXTENSIONFORHTMLFILE

@ayekat
Copy link

ayekat commented Nov 24, 2017

Ah, I see. That was too short-sighted from my side, I apologise.

I assume that CONSTANTVARIABLE... would thus act as a sort of "flag" to help Makefile authors spot variables that should not be reassigned values. Elegant.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants