Enforce code deprecation and virtual methods #1629

mikee47 · 2019-02-17T12:19:31Z

Use SMING_DEPRECATED attribute to ensure marked code not referenced within framework
Apply override to all inherited virtual methods
Ensure base virtual classes have virtual destructors
Remove virtual from overriden destructors
Remove empty overridden destructors and overriden methods that do not change base behaviour
Ensure consistent ordering of constructors/destructor in header

Some samples use deprecated methods. If no suitable alternative has yet been provided, those methods have been un-deprecated and noted in the method comment using @todo. The follow methods have been un-deprecated completely:

`HttpResponse::sendTemplate()`
`WebsocketConnection::operator==(WebsocketConnection&)`

Additional style changes:

Move trivial code into headers
Use nullptr instead of NULL or 0 where appropriate
Remove use of __forceinline or inline where there is no specific need
Reduce sample applications #include <SmingCore/*> to #include <*>
Fix ill-formed doxygen comments
Fix doxygen build error and include SSL code in output

Closes #1620

…t warnings

Ensure base virtual classes have virtual destructors Remove `virtual` from overriden destructors Remove empty overridden destructors Ensure consistent ordering of constructors/destructor in header

…ult to `nullptr` not `0`

…ant inlines

mikee47 · 2019-02-17T16:19:49Z

@slaff As this PR deals with coding style changes I've gone further and updated all the core files to standardise comment headers and that #include guards are named correctly. It affects unrelated files though so I'll assume it's a separate PR; I can commit here if you want.

slaff · 2019-02-18T07:23:28Z

It affects unrelated files though so I'll assume it's a separate PR; I can commit here if you want.

As long as the unrelated files are not part of third-party libraries or Arduino libraries it should be fine to add it also here. But before you do this can you explain what you mean by 'unrelated files` ?

mikee47 · 2019-02-18T07:26:02Z

unrelated files meaning files that otherwise wouldn't be modified as a result of deprecate/override

slaff · 2019-02-18T07:26:47Z

unrelated files meaning files that otherwise wouldn't be modified as a result of deprecate/override

Separate PR please.

slaff

Fantastic work! Only small indentation changes needed.

slaff · 2019-02-18T07:39:13Z

Sming/SmingCore/HardwarePWM.h

@@ -64,8 +68,14 @@ class HardwarePWM
     *  @param  duty Value of duty cycle to set pin to
     *  @param  update Update PWM output
     *  @retval bool True on success
+	 *  @note   This function is used to set the pwm duty cycle for a given pin. If parameter 'update' is false


Please, indent the next two lines with the one above.

slaff · 2019-02-18T07:49:42Z

Sming/Wiring/Printable.h

 class Printable
 {
  public:
+	virtual ~Printable() { }


The indentation needs fixing.

slaff · 2019-02-18T07:52:34Z

Sming/system/include/esp_systemapi.h

@@ -32,6 +32,9 @@
 */
 #define SMING_UNUSED  __attribute__((unused))

+/* Flags a compiler warning when Sming framework methods, functions or types are changed */
+#define __deprecated __attribute__((deprecated))


The other definitions of attributes are uppercase. Can you make also this one to be __DEPRECATED or SMING_DEPRECATED ?

I like SMING_DEPRECATED, it's specific

mikee47 · 2019-02-18T11:08:39Z

Sming/SmingCore/Network/Http/Websocket/WebsocketConnection.h

@@ -121,9 +120,10 @@ class WebsocketConnection
 	void* getUserData();

 	// @deprecated
-	bool operator==(const WebsocketConnection& rhs) const;
+	bool operator==(const WebsocketConnection& rhs) const __deprecated;


@slaff You deprecated this method on 04/05/2017, and enforcing deprecation means there's a corresponding change in the HttpServer_WebSockets sample and possibly user apps; Keep it deprecated?

You deprecated this method on 04/05/2017, and enforc ....

Hm... cannot remember why. You can un-deprecate it if needed.

Rename `__deprecated` to `SMING_DEPRECATED` Fix indenting Tidy #includes for all files modified by this PR Revert include guard renaming (separate PR) Revert changes to files not related to deprecate/override (separate PR) Revert deprecation of `WebsocketConnection::operator==` Ensure `@deprecated` usage resides within doxygen comment Fix doxygen error and include SSL in output

slaff · 2019-02-18T13:49:18Z

@mikee47 Ready to merge?

slaff · 2019-02-18T14:28:37Z

Sming/SmingCore/Network/Http/HttpRequestAuth.h

 	{
 	}
 };

 class HttpBasicAuth : public AuthAdapter
 {
 public:
-	HttpBasicAuth(const String& username, const String& password);
+	HttpBasicAuth(const String& username, const String& password)


@mikee47 You can initialize the variables in initialization list

slaff · 2019-02-18T14:28:51Z

Sming/SmingCore/Network/Http/HttpRequestAuth.h

@@ -47,9 +50,16 @@ class HttpBasicAuth : public AuthAdapter
 class HttpDigestAuth : public AuthAdapter
 {
 public:
-	HttpDigestAuth(const String& username, const String& password);
+	HttpDigestAuth(const String& username, const String& password)


Here too: variables in initialization list

mikee47 · 2019-02-18T16:17:59Z

Sming/SmingCore/Network/Http/HttpRequest.h

@@ -56,7 +56,8 @@ class HttpRequest
 		return new HttpRequest(*this);
 	}

-	HttpRequest& operator=(const HttpRequest& rhs);
+	/** @deprecated Please use `clone()` instead */
+	HttpRequest& operator=(const HttpRequest& rhs) SMING_DEPRECATED;


The copy operator provided is actually unsafe as it doesn't take copies of allocated member variables so I've marked it deprecated. I could go further and make it private, which would prevent applications building if it's used.

See also #1601 (comment). I think probably get rid of the warning in the code and document the copy constructor instead.

Thoughts?

I could go further and make it private, which would prevent applications building if it's used.

For now deprecate it only.

I think probably get rid of the warning in the code and document the copy constructor instead.

Sounds good.

Remove code from copy operator

mikee47 · 2019-02-18T17:10:54Z

Just noticed there's a bunch of overrides missing. Will add those.

…al method and update comments

mikee47 · 2019-02-19T06:56:16Z

@slaff I'm inclined to revert the last change relating to HttpRequest copy constructor, deal with it separately. It's a whole issue with copying HashMap objects.

slaff · 2019-02-19T07:44:13Z

The build is failing:

SmingCore/Network/Http/HttpRequest.cpp: In copy constructor 'HttpRequest::HttpRequest(const HttpRequest&)':
SmingCore/Network/Http/HttpRequest.cpp:19:93: error: use of deleted function 'HttpParams::HttpParams(const HttpParams&)'
 ri), method(value.method), headers(value.headers), postParams(value.postParams)
                                                                               ^
In file included from SmingCore/Network/Http/HttpRequest.h:24:0,
                 from SmingCore/Network/Http/HttpRequest.cpp:13:
SmingCore/Network/Http/HttpParams.h:30:7: note: 'HttpParams::HttpParams(const HttpParams&)' is implicitly deleted because the default definition would be ill-formed:
 class HttpParams : public HashMap<String, String>, public Printable
       ^
In file included from SmingCore/Network/Http/HttpHeaders.h:23:0,
                 from SmingCore/Network/Http/HttpResponse.h:18,
                 from SmingCore/Network/Http/HttpRequestAuth.h:16,
                 from SmingCore/Network/Http/HttpRequest.h:18,
                 from SmingCore/Network/Http/HttpRequest.cpp:13:
Wiring/WHashMap.h:321:5: error: 'HashMap<K, V>::HashMap(const HashMap<K, V>&) [with K = String; V = String]' is private
     HashMap(const HashMap<K, V>& that);
     ^
In file included from SmingCore/Network/Http/HttpRequest.h:24:0,
                 from SmingCore/Network/Http/HttpRequest.cpp:13:
SmingCore/Network/Http/HttpParams.h:30:7: error: within this context
 class HttpParams : public HashMap<String, String>, public Printable
       ^
make[3]: *** [out/build/SmingCore/Network/Http//HttpRequest.o] Error 1
make[3]: Leaving directory `/home/travis/build/SmingHub/Sming/Sming'

I'm inclined to revert the last change relating to HttpRequest copy constructor, deal with it separately

Yes, do it like this. This PR is already quite big.

…- requires separate PR

mikee47 · 2019-02-19T07:47:07Z

This PR is already quite big.

Yes, and there shouldn't be any functional changes in this PR. (Well, intentionally anyway!)

mikee47 · 2019-02-19T10:42:13Z

Coding style rules updated

slaff · 2019-02-19T11:03:59Z

Coding style rules updated

Thanks for the changes. I have read the new text and all looks very good to me.

…mingHub#1629

* Fix memory leaks in `HttpConnection` Change `waitingQueue` from un-owned `RequestQueue*` to `RequestQueue` Free request if queue is full in `send(HttpRequest*)` * Tidy up * Code out of header * Merge `HttpClientConnection` into `HttpConnection` * HttpConnection -> HttpClientConnection * HttpConnectionBase -> HttpConnection * Move `isActive()` from `HttpClientConnection` into `HttpConnection` - applicable to both server and client * Resolve `HttpClientConnection` methods temporarily un-deprecated in #1629 * Revise `HttpConnection` so it can be used with either client or server Move `response` member variable into `HttpConnection` as both client and server have this. Move `getResponse()` method from `HttpClientConnection` into `HttpConnection` Add virtual `getRequest()` method to HttpConnection Move applicable methods from `HttpClientConnection` into `HttpConnection` Note: `send(HttpRquest*)` made virtual in previous commit * Fix comments * Fix deprecated method calls * Don't initialise `userData` in `WebsocketConnection` constructor * Un-deprecate `WebsocketConnection::getActiveWebsockets()` and make `static`, returning `const WebsocketList` to prevent dangerous modifications. See #1469

mikee47 added 7 commits February 16, 2019 15:11

Define __deprecated macro and use to ensure framework builds withou…

2781e98

…t warnings

Apply override directive to inherited virtual methods.

105ca0a

Ensure base virtual classes have virtual destructors Remove `virtual` from overriden destructors Remove empty overridden destructors Ensure consistent ordering of constructors/destructor in header

Fix sample build errors

52689ef

Code into headers, revise deprecation comments, delegates should defa…

4bb4b7f

…ult to `nullptr` not `0`

Fix doxygen comments, tidy #includes, code into headers, remove rednd…

1fb812a

…ant inlines

Reduce sample applications #include <SmingCore/*> to #include <*>

d327bcf

Fix build error

5376197

slaff requested changes Feb 18, 2019

View reviewed changes

mikee47 commented Feb 18, 2019

View reviewed changes

slaff added this to the 3.7.2 milestone Feb 18, 2019

A few __forceinline changes, HttpRequestAuth code into header

ccf1165

slaff reviewed Feb 18, 2019

View reviewed changes

slaff added the 3 - Review label Feb 18, 2019

Fix codacy advisories, deprecate HttpRequest copy operator

b18982a

mikee47 force-pushed the fix/deprecated-override branch from 11ddb12 to b18982a Compare February 18, 2019 15:54

mikee47 commented Feb 18, 2019

View reviewed changes

Remove warning from HttpRequest copy constructor and document instead

9205b48

Remove code from copy operator

mikee47 added 5 commits February 18, 2019 17:39

Add missing virtual -> override changes

522c32e

Remove comment code from default parameters

48382df

Code into header (HttpServerConnection)

bad0056

Revert ReadWriteStream::write(const uint8_t*, size_t) to pure virtu…

eda362a

…al method and update comments

Codacy advisories

2d6dce1

Revert changes related to HashMap copying (HttpHeaders, HttpRequest) …

e425e8b

…- requires separate PR

slaff merged commit 49a9c6a into SmingHub:develop Feb 19, 2019

slaff removed the 3 - Review label Feb 19, 2019

mikee47 deleted the fix/deprecated-override branch February 19, 2019 08:56

mikee47 added a commit to mikee47/Sming that referenced this pull request Feb 24, 2019

Resolve HttpClientConnection methods temporarily un-deprecated in S…

58cdc36

…mingHub#1629

mikee47 added a commit to mikee47/Sming that referenced this pull request Feb 25, 2019

Resolve HttpClientConnection methods temporarily un-deprecated in S…

b6b6317

…mingHub#1629

mikee47 added a commit to mikee47/Sming that referenced this pull request Feb 25, 2019

Resolve HttpClientConnection methods temporarily un-deprecated in S…

a439225

…mingHub#1629

slaff mentioned this pull request Feb 27, 2019

Preparation for Release 3.8.0 #1581

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce code deprecation and virtual methods #1629

Enforce code deprecation and virtual methods #1629

mikee47 commented Feb 17, 2019 •

edited

Loading

mikee47 commented Feb 17, 2019 •

edited

Loading

slaff commented Feb 18, 2019

mikee47 commented Feb 18, 2019

slaff commented Feb 18, 2019

slaff left a comment

slaff Feb 18, 2019

slaff Feb 18, 2019

slaff Feb 18, 2019

mikee47 Feb 18, 2019

mikee47 Feb 18, 2019

slaff Feb 18, 2019 •

edited

Loading

slaff commented Feb 18, 2019

slaff Feb 18, 2019

slaff Feb 18, 2019

mikee47 Feb 18, 2019 •

edited

Loading

slaff Feb 18, 2019

mikee47 commented Feb 18, 2019

mikee47 commented Feb 19, 2019 •

edited

Loading

slaff commented Feb 19, 2019

mikee47 commented Feb 19, 2019 •

edited

Loading

mikee47 commented Feb 19, 2019

slaff commented Feb 19, 2019

Enforce code deprecation and virtual methods #1629

Enforce code deprecation and virtual methods #1629

Conversation

mikee47 commented Feb 17, 2019 • edited Loading

mikee47 commented Feb 17, 2019 • edited Loading

slaff commented Feb 18, 2019

mikee47 commented Feb 18, 2019

slaff commented Feb 18, 2019

slaff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slaff Feb 18, 2019 • edited Loading

Choose a reason for hiding this comment

slaff commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 Feb 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 commented Feb 18, 2019

mikee47 commented Feb 19, 2019 • edited Loading

slaff commented Feb 19, 2019

mikee47 commented Feb 19, 2019 • edited Loading

mikee47 commented Feb 19, 2019

slaff commented Feb 19, 2019

mikee47 commented Feb 17, 2019 •

edited

Loading

mikee47 commented Feb 17, 2019 •

edited

Loading

slaff Feb 18, 2019 •

edited

Loading

mikee47 Feb 18, 2019 •

edited

Loading

mikee47 commented Feb 19, 2019 •

edited

Loading

mikee47 commented Feb 19, 2019 •

edited

Loading