-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Documentation updates for internal constructors and function signature #3365
Documentation updates for internal constructors and function signature #3365
Conversation
97fd996
to
c008403
Compare
docs/contracts.rst
Outdated
Constructor functions can be either ``public`` or ``internal``. | ||
|
||
:: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verison pragma is missing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axic for the version pragma, do we specify the minimum version required to run the sample or simply the current version, i.e. 0.4.19
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far the rule has been the minimum required by the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release notes aren't very clear on when this was introduced but Remix thinks this breaks from 0.4.10 downwards so I'll use pragma min 0.4.11 in the example.
From release notes on 0.4.11:
Type system: Contract inheriting from base with unimplemented constructor should be abstract.
docs/contracts.rst
Outdated
:: | ||
|
||
contract A { | ||
uint public a; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Identation is off here
c008403
to
6cfbdec
Compare
docs/contracts.rst
Outdated
function B() public {} | ||
} | ||
|
||
A constructor set as ``internal`` causes the contract to be marked as :ref:`abstract <abstract-contract>`. Child contracts of an abstract contract can call the super constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'd extend this to say that when extending a contract the parent constructor is called implicitly an as such it is possible that a child contract inheriting an internal
constructor from the parent makes it non-abstract. (Would need to phrase this in a more readable form.)
Then as a second feature to that of course one can call the super constructor even if it is marked internal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is specific to the description of an abstract contracts I think I'll add it to that section if you don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axic I don't get what you mean here:
child contract inheriting an internal constructor from the parent makes it non-abstract
A child contract inherits an abstract contract with internal constructor does not become non-abstract necessarily as if for instance it doesn't implement all of the abstract functions, it will be abstract.
502bc38
to
c8342df
Compare
c8342df
to
1541d1d
Compare
docs/contracts.rst
Outdated
Abstractions | ||
************ | ||
|
||
An abstraction type describes a contract but does not provide a full implementation of the contract. An abstraction is implemented as abstract contract or interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what "abstraction type" means here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where the confusion comes from, since there's no actual abstract type in Solidity it's probably best to rename "abstraction type" to just "abstraction"
docs/abi-spec.rst
Outdated
@@ -26,6 +26,10 @@ The first four bytes of the call data for a function call specifies the function | |||
first (left, high-order in big-endian) four bytes of the Keccak (SHA-3) hash of the signature of the function. The signature is defined as the canonical expression of the basic prototype, i.e. | |||
the function name with the parenthesised list of parameter types. Parameter types are split by a single comma - no spaces are used. | |||
|
|||
.. note:: | |||
Return type of the function is not taken into account. In :ref:`function overloading <overload-function>` return types are not considered. The reason is to keep function call resolution context-independent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd clarify the link as "In Solidity's function overloading". The ABI documentation is not Solidity specific, we have just "hijacked it" and imported into our documentation :)
docs/contracts.rst
Outdated
.. _abstractions: | ||
|
||
************ | ||
Abstractions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this section introduce this term? If yes, is it intended to capture both abstract base contracts and interfaces? I'm not sure we should do this as it might be too easily confused with "abstract contract". I would say an interface is a special kind of abstract base contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @chriseth see the PR description for motivation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm not sure I understand the difference you're drawing between an "abstract contract" and an "abstract base contract", can you explain? An abstract contract cannot be instantiated and therefore makes no sense unless it's a base contract too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need this section in its current form.
How about having a title like "Special contracts" / "Special contract kinds" / "Special contract types" (or something better you can come up with)?
It would only need a sentence stating that some contracts cannot be compiled, but can be used as bases for other contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the common section was not a good idea after all so I've dropped it
docs/contracts.rst
Outdated
@@ -1051,13 +1086,13 @@ but they can be used as base contracts:: | |||
function utterance() public returns (bytes32) { return "miaow"; } | |||
} | |||
|
|||
Abstract contracts can implement most of a contract, but leave some methods abstract, facilitating patterns like template method and removing code duplication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what the template method is?
Also, to remove code duplication, it is not required to use an abstract contract, a base contract is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wikipedia does a good job of explaining the template method, we've used wiki sources before so I assume it's ok to reference that instead of try to redefine it? https://en.wikipedia.org/wiki/Template_method_pattern
d265047
to
cb23a2a
Compare
docs/contracts.rst
Outdated
@@ -1051,13 +1086,13 @@ but they can be used as base contracts:: | |||
function utterance() public returns (bytes32) { return "miaow"; } | |||
} | |||
|
|||
Abstract contracts can implement most of a contract, but leave some methods abstract, facilitating patterns like `Template method <https://en.wikipedia.org/wiki/Template_method_pattern>`_ and removing code duplication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps "leave some methods unimplemented" is less confusing since there is no way to mark a method "abstract".
9069be2
to
eee50c2
Compare
docs/abi-spec.rst
Outdated
@@ -26,6 +26,10 @@ The first four bytes of the call data for a function call specifies the function | |||
first (left, high-order in big-endian) four bytes of the Keccak (SHA-3) hash of the signature of the function. The signature is defined as the canonical expression of the basic prototype, i.e. | |||
the function name with the parenthesised list of parameter types. Parameter types are split by a single comma - no spaces are used. | |||
|
|||
.. note:: | |||
Return type of the function is not taken into account. In :ref:`Solidity's function overloading <overload-function>` return types are not considered. The reason is to keep function call resolution context-independent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps The return type of a function is not part of this signature.
docs/contracts.rst
Outdated
|
||
Constructors | ||
============ | ||
Constructor is an optional function with the same name as the contract which is executed upon contract creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps: A constructor is an optional...
docs/contracts.rst
Outdated
@@ -1065,6 +1090,8 @@ Example of a Function Type (a variable declaration, where the variable is of typ | |||
|
|||
function(address) external returns (address) foo; | |||
|
|||
Abstract contracts allow you to decouple contract definition from implementation providing better extensibility and self-documentation and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps decouple the definition of a contract from its implementation
?
docs/contracts.rst
Outdated
@@ -1065,6 +1090,8 @@ Example of a Function Type (a variable declaration, where the variable is of typ | |||
|
|||
function(address) external returns (address) foo; | |||
|
|||
Abstract contracts allow you to decouple contract definition from implementation providing better extensibility and self-documentation and | |||
facilitating patterns like `Template method <https://en.wikipedia.org/wiki/Template_method_pattern>`_ and removing code duplication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the Template method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that this pull request took so long. If you do the final small changes, we can merge it.
eee50c2
to
4531a97
Compare
np @chriseth I understand you're busy. Changes done and rebased |
Closes #3131
Closes #3132
Closes #627