https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py File contrib/profile_merge.py (right):
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode53 contrib/profile_merge.py:53: data_file = open(path, 'rb') On 2013/04/09 17:32:11, martint wrote:
How about simpler version:
with open(file, 'rb') as f: data = f.read()
Done. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode59 contrib/profile_merge.py:59: def MergeCounters(objs, index, multipliers): changed the name to ReturnMergedCounters On 2013/04/09 17:32:11, martint wrote:
Perhaps use function name that shows that it returns the value.
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode78 contrib/profile_merge.py:78: length: Length of the data on the disk All the bullets sub-to Attributes are not capital. On 2013/04/09 17:32:11, martint wrote:
Minor style. Make sure format is coherent (ends with . or not, starts
with
capital letter or not).
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode119 contrib/profile_merge.py:119: self.ident = 0 Probably not. 'reader' here is ProfileDataFile. We should have a valid gcda/gcno file for this program. But it does not hurt to have a empty reader. I'll leave as it's. On 2013/04/09 17:32:11, martint wrote:
It is ever useful to have a function object with no reader?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode138 contrib/profile_merge.py:138: return self.ArcCounters().counters[0] that will not happen. all functions have at least one counter. On 2013/04/09 17:32:11, martint wrote:
This will throw an exception if no counters exists. Is that expected?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode209 contrib/profile_merge.py:209: class Lines(DataObject): It's for reading in gcno file. We are not using this functionality for merge. On 2013/04/09 17:32:11, martint wrote:
Where is this used?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1153 contrib/profile_merge.py:1153: os.chdir(direc) On 2013/04/09 17:32:11, martint wrote:
How about using absolute paths to avoid the chdir logic?
Done. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1199 contrib/profile_merge.py:1199: outfile_path = output_path + '/' + f On 2013/04/09 17:32:11, martint wrote:
os.path.join() instead of +, here and other places where the pattern
is used. Done. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1213 contrib/profile_merge.py:1213: def Merge(self, archives, multipliers): The caller in this program always has self in archives set. On 2013/04/09 17:32:11, martint wrote:
The code seems to handle both the case when 'self' is inside archives
and when
it is not. Does both happen when running the code?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1233 contrib/profile_merge.py:1233: for i, a in enumerate(archives): i is the i-th profile archive. On 2013/04/09 17:32:11, martint wrote:
It's hard to see what i is in this case. I would prefer to have
information
about arguments in the documentation at the top of the function, but
more
descriptive variable names and comments would also help.
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1313 contrib/profile_merge.py:1313: input_profiles[0].Merge(input_profiles, profile_multipliers) That will use more memory. We already use too much memory. And it only modifies in-memory object (the profile files are not changed). On 2013/04/09 17:32:11, martint wrote:
Did you conside creating a new object instead of modifying the
existing? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1317 contrib/profile_merge.py:1317: input_profiles[0].Write(output_profile) Refer to previous comment. Only in-memory objects are overwritten. On 2013/04/09 17:32:11, martint wrote:
Warn before overwriting?
https://codereview.appspot.com/8508048/
