Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:119300 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 23811 invoked from network); 18 Jan 2023 08:45:56 -0000 Received: from unknown (HELO php-smtp4.php.net) (45.112.84.5) by pb1.pair.com with SMTP; 18 Jan 2023 08:45:56 -0000 Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id 6A0F7180212 for ; Wed, 18 Jan 2023 00:45:55 -0800 (PST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on php-smtp4.php.net X-Spam-Level: X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,HTML_MESSAGE, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.2 X-Spam-ASN: AS15169 209.85.128.0/17 X-Spam-Virus: No X-Envelope-From: Received: from mail-ed1-f52.google.com (mail-ed1-f52.google.com [209.85.208.52]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by php-smtp4.php.net (Postfix) with ESMTPS for ; Wed, 18 Jan 2023 00:45:54 -0800 (PST) Received: by mail-ed1-f52.google.com with SMTP id 18so48610966edw.7 for ; Wed, 18 Jan 2023 00:45:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=sj3R0F0NnqOdZgok4nOVBXj5U7pv3byxJPHX6Iphgew=; b=iyzOVBVvJTH2jqb+xwfpJ6M7MpOrmv6IVg0L4CXG6cspFMR0PKLfRFubrzx0dXlQNc mMyaCRYuUqJ6615z0L5MdBG0pLSUGFaEYetZc+1SJBpQcLmQVdD1BMn7x6IWBrZ9JuOa Bm9qXaqAnpxbiwHA6RFJPMab6V9KTOoOECzopRyXPPvJ0ZpFGQLHI5c62nvJw5fo9McJ CdhsHkIDOeEWzCI9JxohpYqpgHgpVh9fpkBWqZzvAm54lIU2g+UoYYmTjAjPJIr9/1lW 7uQuaRCgPNryPHRZsO1xrZqgENdOaPDNeWrv66mz8Te3VSEvFU0xB/4P4s55FjAPwLXD zXbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=sj3R0F0NnqOdZgok4nOVBXj5U7pv3byxJPHX6Iphgew=; b=4FoEerDdc9PC2QKg4bq7kyUvyyxUp66lOaUa+l9felRceOu7lMaIpJR8qvyNS1wjth nHqrF5dUvbh1mfTU8rt/AmiIz3qoiM979PFk1RUyAcy10Uq4qnaSpE+msPXeOEj7zw6a XB00fSq6c7docSizBhtWmmUgoFnS55Pqf9//NSHEjwJeG3JCVGy++iFO8ziZlpFTlXnn OQX4wCAU85SBnvu/N2iueuSoNUQqXj127b7oGJLVcjXs7l/jeEAbjB32vZCDdVbHIjeo uHxZtcYm3UkOOg/cfILS4m7fcc6C0ar1cR1ytFZ/pBY5iCU4HFtXzc+BN+dfoJJhc3Fu qOEA== X-Gm-Message-State: AFqh2kr5YVJKY08OV5ZPJ+uj0Ng97+2/MLj6fLRbPmH/Xbtzz2FpfqjE 00/6JTm4QPFGi4xFd6z4fVXqdKQmAEXufptKijPpO+EMHAc= X-Google-Smtp-Source: AMrXdXu2Gn7+QUks2bXE1QIHwKl/uX+o9Ulzfb6aZ00gGWd2crvsQINk56u+tExrvbOfh+06JfZGwNzUgcRbGmsPWhs= X-Received: by 2002:a05:6402:28a4:b0:485:2bdf:ca28 with SMTP id eg36-20020a05640228a400b004852bdfca28mr924981edb.251.1674031553521; Wed, 18 Jan 2023 00:45:53 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: Date: Wed, 18 Jan 2023 11:45:42 +0300 Message-ID: To: Levi Morrison Cc: Max Kellermann , internals@lists.php.net Content-Type: multipart/alternative; boundary="00000000000017445005f285d8e2" Subject: Re: [PHP-DEV] RFC: rules for #include directives From: dmitrystogov@gmail.com (Dmitry Stogov) --00000000000017445005f285d8e2 Content-Type: text/plain; charset="UTF-8" Here are my comments about the last PR https://github.com/php/php-src/pull/10345 Max asked to repost them in this thread: In my opinion this should not be accepted. Despite the intention is good, this huge patch doesn't improve anything but adds new questions. - It introduces new include files (e.g. zend_char.h, what was wrong with zend_portability.h?) - It adds a lot of forward declarations (e.g. typedef struct _ ) - It adds almost useless comments that are not goigg to be kept as to date (e.g. #include "zend_portability.h" // for BEGIN_EXTERN_C, zend_portability.h defines all base macros for Zend engine) - zend.c adds a number of commented includes (is this some kind of TODO?) - > is included in many other *.h files instead of single zend_portability.h or zend_types.h. I see all these changes as a source permutation that don't add significant value. On the other hand: - this may introduce PHP build failures on some untested configurations (like it was with DTRACE) - we have to support several PHP branches at once and merging fixes between very different branches is going to introduce problems - this changes may break compilation of third-party software (e.g. xdebug). Not a huge problem, but it's not great to do this in a minor version update. On Mon, Jan 16, 2023 at 8:22 PM Levi Morrison via internals < internals@lists.php.net> wrote: > As commented yesterday on one of the PRs, I strongly agree with the > cleanups. Our code has implicit dependencies and if you reorder the > includes in the same file, you can break builds. That shouldn't ever > happen. > > I also agree that dtrace breaking is a failure of CI and testing, not > necessarily the patch (I haven't reviewed that PR carefully, maybe > there was some carelessness but if it's not tested, it's hard to blame > anyone). > > Lastly, I do not care for the comments which explain why a header is > included. My experience is that these comments bitrot quite quickly. > > Overall, I hope we can resume this effort. > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > > --00000000000017445005f285d8e2--