Details
Diff Detail
Event Timeline
The overall structure seems alright. I'll let people more familiar with the code-base judge whether other important features should be added there as well.
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 49 | "Full experimental" feels like an oxymoron. I would drop the "full". | |
| 49 | "support of" -> "support for" | |
| 138 | Support for address space attributes [...] improved (refer [...] Alternatively, if you don't want to use parenthesis, drop the closing parenthesis on the next line. | |
| 142 | Inconsistent sentence capitalization between the two bullet points. | |
| 173 | If the BIF acronym wasn't introduced before, it should be replaced with "builtin functions". It seems we don't have more file context in this review so I cannot tell. | |
| 175 | I'm not 100% sure about the grammar rule in English, but shouldn't there be a "-" between "frontend" and "only" here to make it an adjective-ish? | |
| 183 | Simplified the representation of blocks, including their generation into IR. Furthermore, indirect calls [...] (I'm assuming here that the indirect calls to block function is not the only improvement. And even if it is, "i.e." is less impressive, isn't it?) | |
| 194 | Maybe this would be better? Improved math builtin functions with parameters of type long long for x86. | |
| 201 | Ditto, I would drop "full" here. | |
| 202 | compatible with | |
| 209 | Did you meant to indent this bullet point as well? | |
| 215 | Missing comma: "functions, including" | |
| 220 | Maybe something along these line would be better? Methods can be overloaded for different address spaces. Or, if you want to emphasis the qualifiers, Method qualifiers now include address space. | |
| 222 | This seems to be already included in previous point. | |
| 226 | Which "cast" operators? | |
| 226 | [...] are now prevented from converting [...] | |
| 229 | missing comma: "C, including" | |
| 232–233 | I would suggest this: OpenCL-specific types, except within blocks: [...] | |
| 237–238 | Stab? Stub? Global constructors can be invoked from the host side using a specific, compiler-generated kernel. | |
| 240–241 | [...] to all atomic builtins(?), including the ones prior to [...] | |
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 173 | BIFs -> built-in functions | |
| 175 | The flag is called -fdeclare-opencl-builtins (not -fadd...). | |
| 176 | The option does not only "enable a trie" during parsing. I'd suggest to just drop "to enable trie during parsing". | |
| 179 | Refactored the opencl-c.h header file ... TableGen trie -> -fdeclare-opencl-builtins. | |
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 173 | builtin is a more common spelling in Clang docs. | |
| 176 | We don't have to describe all details here btw, but just the main ones. I would like to say what the flag is used for explicitly rather than leaving the room for interpretations. Btw, would it make sense to document this flag in UserManual? https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options | |
| 220 | I prefer the second one, as the first one can be read as parameter address space. | |
| 222 | Do you mean method qualifiers? I don't see the connection? | |
| 232–233 | I mean with the exception of blocks here. | |
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 209 | I think Sphinx/RST wants an empty line here. Nitpicking for consistency, could you have a ; at then end of each bullet point (except the last one, obviously)? Thanks. | |
lgtm, and many thanks for writing release notes!
Go ahead and commit to the branch directly with SVN or let me know if you'd like me to do it.
"Full experimental" feels like an oxymoron. I would drop the "full".