[PATCH] D46485: Add python tool to dump and construct header maps

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Bruno, I just noticed couple of implementation details.



================
Comment at: utils/hmaptool/hmaptool:55
+            # The number of buckets must be a power of two.
+            if num_buckets == 0 or (num_buckets & num_buckets - 1) != 0:
+                raise SystemExit("error: %s: invalid number of buckets" % (
----------------
Wouldn't it be simpler to use modulo 2?

```
if num_buckets % 2 == 0
```


================
Comment at: utils/hmaptool/hmaptool:153
+def next_power_of_two(value):
+    for i in range(32):
+        power = 1 << i
----------------
This is probably not critically important but maybe we could use math functions instead of iteration here:


```
from math import *
return ceil( log( value + 1, 2) )
```

...or if we want to support values <1:
```
if value <= 0
    raise ArgumentError
if value < 1
    return 0

return ceil( log( value, 2) ) + 1
```


================
Comment at: utils/hmaptool/hmaptool:234
+
+    if len(args) != 2:
+        parser.error("invalid number of arguments")
----------------
Aren't we expecting just single argument (<headermap path>) here?


Repository:
  rC Clang

https://reviews.llvm.org/D46485