Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

JANITORIAL: Replace new[]/memset with new[]() #2820

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@orgads
Copy link
Contributor

@orgads orgads commented Mar 7, 2021

Mostly done using the following Ruby script:

(Dir.glob('**/*.cpp') + Dir.glob('**/*.h')).each do |file|
  s = File.read(file, encoding: 'iso8859-1')
  t = s.gsub(/(([\w_.\[\]]+)\s*=\s*new\s+\S+?\[[^\]]+?\](?!\())([^\{\}]*?)\n\s+memset\(\s*\2\s*,\s*0\s*,[^;]*;/m, '\1()\3')
  if t != s
    File.open(file, 'w') { |io| io.write(t) }
  end
end
@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Mar 8, 2021

This is a C++03 construct while ScummVM is restricted to a C++98 subset according to the Wiki rules

@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 8, 2021

I see that it's already used in ags, bladerunner, dm, scumm and wintermute. Should these be also changed to use memset instead?

Search pattern: git grep new.*\]\(\)

@digitall
Copy link
Member

@digitall digitall commented Mar 8, 2021

@orgads : Ignoring for a second the merits and disadvantages of this change, I would highly recommend splitting this into a commit per engine and subsystem i.e. "JANITORIAL: AGOS: Replace new[]/memset with new" etc. as this makes it easier to review the changes for engine authors and makes future bisection for any regressions a bit less problematic i.e. too many changes in a single commit which you have to split apart to locate the hunk causing the issue.

@digitall
Copy link
Member

@digitall digitall commented Mar 8, 2021

If the changes were related i.e. in a Common API change, then this would need to be a singular commit to avoid unbuildable commits and propagate the change, but these changes are not inter-related and could be split.

@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 8, 2021

I agree. I considered splitting it, but didn't know what will you prefer. Anyway, since it is likely to break older compilers, I'll close it. Thanks.

@orgads orgads closed this Mar 8, 2021
@orgads orgads reopened this Oct 13, 2021
Mostly done using the following Ruby script:

(Dir.glob('**/*.cpp') + Dir.glob('**/*.h')).each do |file|
  s = File.read(file, encoding: 'iso8859-1')
  t = s.gsub(/(([\w_.\[\]]+)\s*=\s*new\s+\S+?\[[^\]]+?\](?!\())([^\{\}]*?)\n\s+memset\(\s*\2\s*,\s*0\s*,[^;]*;/m, '\1()\3')
  if t != s
    File.open(file, 'w') { |io| io.write(t) }
  end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants