Skip to content

Fix some shadowing by using more distinct variable names#228

Open
0Xellos wants to merge 2 commits into
MikeMirzayanov:masterfrom
0Xellos:fix-some-shadowing
Open

Fix some shadowing by using more distinct variable names#228
0Xellos wants to merge 2 commits into
MikeMirzayanov:masterfrom
0Xellos:fix-some-shadowing

Conversation

@0Xellos
Copy link
Copy Markdown

@0Xellos 0Xellos commented Apr 20, 2026

Fixing some -Wshadow warnings, mostly renaming, one redundant argument removed. Reasoning copied from blog

  • In the class pattern, there's a member s (line 733) that's shadowed by a c.arg. (line 1531). However, the constructor doesn't just use copy s to pattern::s, it also parses it into a more structured form that the new instance stores (note that s is passed by mutable value). Then, s is only used at one point - the function pattern::src returns it. Therefore, let's rename it to source, making the code self-documenting that it's the pattern's source string.
  • The member function FileInputStreamReader::getc (line 1777) takes a FILE* as an argument, which shadows its member file (line 1763). This function is private and only used within that class a few times with the member file passed as that argument everytime. We might as well remove that argument file, it's unnecessary and confusing.
  • Also in the class FileInputStreamReader, there's a member name (line 1764) shadowed in a constructor (line 1809). The same happens in BufferedFileInputStreamReader (lines 1882, 1923). I'm not sure what those variables are supposed to be for and it highlights why it's good to deal with shadowing - when a constructor takes a file and a name, is it supposed to be the name of the file? Or a more general name given to the stream, which would probably include the filename but isn't limited to it? There are only few usages that don't shed much light on the situation, so I'm going to err on the side of generality and rename the c.arg. to streamName in both cases to indicate that it's not forced to be just the filename.
  • The class InStream has a member mode (line 2018) that's shadowed by a c.arg. repeatedly again (lines ~2030, ~3310). What mode? Mode in testlib can be file opening mode, strict/non-strict, input/output/answer, it's quite a confusing word... and the way it's named would suggest some file mode or stream mode, but it's actually input/output/answer (as the type shows, but digging through types shouldn't be action number 1). That has nothing to do with the stream or the file. I want to rename each c.arg. to contentMode and am opening an issue that all these modes should be named clearer.
  • Function setAppesModeEncoding (line 4645) is setting the global variable appesModeEncoding (line 2382) from a shadow argument. Putting aside that global variables are evil and should at least be put in a global state struct within a namespace, I tend to prefix such arguments with new, so newAppesModeEncoding. Sometimes the current value matters and it's better to distinguish between current (not yet old, the change can fail!) and new value by name.

Still remaining: stream readers' member file, members of ValidatorBoundsHit shadowed in constructors are named clearly enough, see #229.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant