Skip to content
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

Silabs #274

Merged
merged 17 commits into from
Oct 29, 2021
Merged

Silabs #274

merged 17 commits into from
Oct 29, 2021

Conversation

tecimovic
Copy link
Collaborator

@tecimovic tecimovic commented Oct 27, 2021

  • Performance enhancements: this shaves of about 25% of generation time for the ZigbeePro stack.
  • Add cascading deletes to many tables that didn't have this set correctly

Comment on lines +78 to +80
.dbGet(db, `${ATOMIC_QUERY} WHERE PACKAGE_REF = ? AND UPPER(NAME) = ?`, [
packageId,
name == null ? name : name.toUpperCase(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in selectAtomicSizeFromType we lowercase the incoming string, and rely on the stored value being lowercase. But here we are uppercasing both. Why the inconsistency?

I guess this is pre-existing, but could we then at least not have the new queries we are adding here do the uppercasing thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a valid point. I simply preserved the previous muddle and added the cache. These queries are not new, they were simply isolated out of query-zcl.js.

Let me think about this a bit... I don't like any of this case-insensitivity.... Pretty much any language environment zap is targeting has case-sensitive type namespaces, and if someone wants to generate COBOL stuff from zap, I guess I'll just open a bottle of beer and cheer for them!

So let me spend a bit of thinking time around how to clean this up for good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to be clear, I am OK with keeping the existing queries; the question is what to do for the caching bits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching bits are initially a bit more promiscuous because we had separate queries that in one case were, and in other case were not case sensitive. What I want to do:

  • clean this whole mess up, and make it strict in ALL cases, after I figure out what the root problem is and probably end up cleaning up the XML files.
    - meanwhile, use these in a maximally promiscuous mode, matching non-case sensitive, since any intermediate solution feels just more messy. Hence the cache being even more promiscuous.

src-electron/db/query-atomic.js Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Oct 27, 2021

CLA assistant check
All committers have signed the CLA.

@tecimovic
Copy link
Collaborator Author

Rebased against master before merge.

@tecimovic tecimovic merged commit f4345e9 into project-chip:master Oct 29, 2021
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this pull request Nov 12, 2021
project-chip/zap#274 should have fixed the problem that caused them to be commented out.

Fixes project-chip#11056
Fixes project-chip#11057
woody-apple pushed a commit to project-chip/connectedhomeip that referenced this pull request Nov 12, 2021
project-chip/zap#274 should have fixed the problem that caused them to be commented out.

Fixes #11056
Fixes #11057
PSONALl pushed a commit to PSONALl/connectedhomeip that referenced this pull request Dec 3, 2021
project-chip/zap#274 should have fixed the problem that caused them to be commented out.

Fixes project-chip#11056
Fixes project-chip#11057
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants